From 950c1cbf8b4de8af7d1933ab9ff25b946ffe3ddc Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 24 Sep 2019 15:05:27 +0200 Subject: [PATCH 01/17] Extract disk serving from domain package --- internal/domain/domain.go | 330 +------------------------- internal/domain/domain_test.go | 23 -- internal/serving/disk/errors.go | 18 ++ internal/serving/disk/group.go | 64 +++++ internal/serving/disk/helpers.go | 77 ++++++ internal/serving/disk/helpers_test.go | 32 +++ internal/serving/disk/project.go | 58 +++++ internal/serving/disk/reader.go | 162 +++++++++++++ internal/serving/handler.go | 7 - internal/serving/serving.go | 30 ++- internal/source/groups/map.go | 17 +- 11 files changed, 459 insertions(+), 359 deletions(-) create mode 100644 internal/serving/disk/errors.go create mode 100644 internal/serving/disk/group.go create mode 100644 internal/serving/disk/helpers.go create mode 100644 internal/serving/disk/helpers_test.go create mode 100644 internal/serving/disk/project.go create mode 100644 internal/serving/disk/reader.go delete mode 100644 internal/serving/handler.go diff --git a/internal/domain/domain.go b/internal/domain/domain.go index f342bd72c..9678f5fca 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -3,32 +3,14 @@ package domain import ( "crypto/tls" "errors" - "fmt" - "io" - "mime" "net/http" - "os" - "path/filepath" - "strconv" - "strings" "sync" "time" - "golang.org/x/sys/unix" - "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" - "gitlab.com/gitlab-org/gitlab-pages/internal/httputil" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" ) -type locationDirectoryError struct { - FullPath string - RelativePath string -} - -type locationFileNoExtensionError struct { - FullPath string -} - // GroupConfig represents a per-request config for a group domain type GroupConfig interface { IsHTTPSOnly(*http.Request) bool @@ -36,7 +18,6 @@ type GroupConfig interface { IsNamespaceProject(*http.Request) bool ProjectID(*http.Request) uint64 ProjectExists(*http.Request) bool - ProjectWithSubpath(*http.Request) (string, string, error) } // Domain is a domain that gitlab-pages can serve. @@ -52,6 +33,7 @@ type Domain struct { AccessControl bool GroupConfig GroupConfig // handles group domain config + Serving serving.Serving certificate *tls.Certificate certificateError error @@ -75,41 +57,6 @@ func (d *Domain) isCustomDomain() bool { return d.GroupConfig == nil } -func (l *locationDirectoryError) Error() string { - return "location error accessing directory where file expected" -} - -func (l *locationFileNoExtensionError) Error() string { - return "error accessing a path without an extension" -} - -func acceptsGZip(r *http.Request) bool { - if r.Header.Get("Range") != "" { - return false - } - - offers := []string{"gzip", "identity"} - acceptedEncoding := httputil.NegotiateContentEncoding(r, offers) - return acceptedEncoding == "gzip" -} - -func handleGZip(w http.ResponseWriter, r *http.Request, fullPath string) string { - if !acceptsGZip(r) { - return fullPath - } - - gzipPath := fullPath + ".gz" - - // Ensure the .gz file is not a symlink - if fi, err := os.Lstat(gzipPath); err != nil || !fi.Mode().IsRegular() { - return fullPath - } - - w.Header().Set("Content-Encoding", "gzip") - - return gzipPath -} - // IsHTTPSOnly figures out if the request should be handled with HTTPS // only by looking at group and project level config. func (d *Domain) IsHTTPSOnly(r *http.Request) bool { @@ -151,20 +98,7 @@ func (d *Domain) HasAcmeChallenge(token string) bool { return false } - _, err := d.resolvePath(d.Project, ".well-known/acme-challenge", token) - - // there is an acme challenge on disk - if err == nil { - return true - } - - _, err = d.resolvePath(d.Project, ".well-known/acme-challenge", token, "index.html") - - if err == nil { - return true - } - - return false + return d.Serving.HasAcmeChallenge(token) } // IsNamespaceProject figures out if the request is to a namespace project @@ -209,234 +143,6 @@ func (d *Domain) HasProject(r *http.Request) bool { return d.GroupConfig.ProjectExists(r) } -// Detect file's content-type either by extension or mime-sniffing. -// Implementation is adapted from Golang's `http.serveContent()` -// See https://github.com/golang/go/blob/902fc114272978a40d2e65c2510a18e870077559/src/net/http/fs.go#L194 -func (d *Domain) detectContentType(path string) (string, error) { - contentType := mime.TypeByExtension(filepath.Ext(path)) - - if contentType == "" { - var buf [512]byte - - file, err := os.Open(path) - if err != nil { - return "", err - } - - defer file.Close() - - // Using `io.ReadFull()` because `file.Read()` may be chunked. - // Ignoring errors because we don't care if the 512 bytes cannot be read. - n, _ := io.ReadFull(file, buf[:]) - contentType = http.DetectContentType(buf[:n]) - } - - return contentType, nil -} - -func (d *Domain) serveFile(w http.ResponseWriter, r *http.Request, origPath string) error { - fullPath := handleGZip(w, r, origPath) - - file, err := openNoFollow(fullPath) - if err != nil { - return err - } - - defer file.Close() - - fi, err := file.Stat() - if err != nil { - return err - } - - if !d.IsAccessControlEnabled(r) { - // Set caching headers - w.Header().Set("Cache-Control", "max-age=600") - w.Header().Set("Expires", time.Now().Add(10*time.Minute).Format(time.RFC1123)) - } - - contentType, err := d.detectContentType(origPath) - if err != nil { - return err - } - - w.Header().Set("Content-Type", contentType) - http.ServeContent(w, r, origPath, fi.ModTime(), file) - - return nil -} - -func (d *Domain) serveCustomFile(w http.ResponseWriter, r *http.Request, code int, origPath string) error { - fullPath := handleGZip(w, r, origPath) - - // Open and serve content of file - file, err := openNoFollow(fullPath) - if err != nil { - return err - } - defer file.Close() - - fi, err := file.Stat() - if err != nil { - return err - } - - contentType, err := d.detectContentType(origPath) - if err != nil { - return err - } - - w.Header().Set("Content-Type", contentType) - w.Header().Set("Content-Length", strconv.FormatInt(fi.Size(), 10)) - w.WriteHeader(code) - - if r.Method != "HEAD" { - _, err := io.CopyN(w, file, fi.Size()) - return err - } - - return nil -} - -// Resolve the HTTP request to a path on disk, converting requests for -// directories to requests for index.html inside the directory if appropriate. -func (d *Domain) resolvePath(projectName string, subPath ...string) (string, error) { - publicPath := filepath.Join(d.Group, projectName, "public") - - // Don't use filepath.Join as cleans the path, - // where we want to traverse full path as supplied by user - // (including ..) - testPath := publicPath + "/" + strings.Join(subPath, "/") - fullPath, err := filepath.EvalSymlinks(testPath) - if err != nil { - if endsWithoutHTMLExtension(testPath) { - return "", &locationFileNoExtensionError{ - FullPath: fullPath, - } - } - - return "", err - } - - // The requested path resolved to somewhere outside of the public/ directory - if !strings.HasPrefix(fullPath, publicPath+"/") && fullPath != publicPath { - return "", fmt.Errorf("%q should be in %q", fullPath, publicPath) - } - - fi, err := os.Lstat(fullPath) - if err != nil { - return "", err - } - - // The requested path is a directory, so try index.html via recursion - if fi.IsDir() { - return "", &locationDirectoryError{ - FullPath: fullPath, - RelativePath: strings.TrimPrefix(fullPath, publicPath), - } - } - - // The file exists, but is not a supported type to serve. Perhaps a block - // special device or something else that may be a security risk. - if !fi.Mode().IsRegular() { - return "", fmt.Errorf("%s: is not a regular file", fullPath) - } - - return fullPath, nil -} - -func (d *Domain) tryNotFound(w http.ResponseWriter, r *http.Request, projectName string) error { - page404, err := d.resolvePath(projectName, "404.html") - if err != nil { - return err - } - - err = d.serveCustomFile(w, r, http.StatusNotFound, page404) - if err != nil { - return err - } - return nil -} - -func (d *Domain) tryFile(w http.ResponseWriter, r *http.Request, projectName string, subPath ...string) error { - fullPath, err := d.resolvePath(projectName, subPath...) - - if locationError, _ := err.(*locationDirectoryError); locationError != nil { - if endsWithSlash(r.URL.Path) { - fullPath, err = d.resolvePath(projectName, filepath.Join(subPath...), "index.html") - } else { - // Concat Host with URL.Path - redirectPath := "//" + r.Host + "/" - redirectPath += strings.TrimPrefix(r.URL.Path, "/") - - // Ensure that there's always "/" at end - redirectPath = strings.TrimSuffix(redirectPath, "/") + "/" - http.Redirect(w, r, redirectPath, 302) - return nil - } - } - - if locationError, _ := err.(*locationFileNoExtensionError); locationError != nil { - fullPath, err = d.resolvePath(projectName, strings.TrimSuffix(filepath.Join(subPath...), "/")+".html") - } - - if err != nil { - return err - } - - return d.serveFile(w, r, fullPath) -} - -func (d *Domain) serveFileFromGroup(w http.ResponseWriter, r *http.Request) bool { - projectName, subPath, err := d.GroupConfig.ProjectWithSubpath(r) - if err != nil { - httperrors.Serve404(w) - return true - } - - if d.tryFile(w, r, projectName, subPath) == nil { - return true - } - - return false -} - -func (d *Domain) serveNotFoundFromGroup(w http.ResponseWriter, r *http.Request) { - projectName, _, err := d.GroupConfig.ProjectWithSubpath(r) - - if err != nil { - httperrors.Serve404(w) - return - } - - // Try serving custom not-found page - if d.tryNotFound(w, r, projectName) == nil { - return - } - - // Generic 404 - httperrors.Serve404(w) -} - -func (d *Domain) serveFileFromConfig(w http.ResponseWriter, r *http.Request) bool { - // Try to serve file for http://host/... => /group/project/... - if d.tryFile(w, r, d.Project, r.URL.Path) == nil { - return true - } - - return false -} - -func (d *Domain) serveNotFoundFromConfig(w http.ResponseWriter, r *http.Request) { - // Try serving not found page for http://host/ => /group/project/404.html - if d.tryNotFound(w, r, d.Project) == nil { - return - } - - // Serve generic not found - httperrors.Serve404(w) -} - // EnsureCertificate parses the PEM-encoded certificate for the domain func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { if !d.isCustomDomain() { @@ -454,42 +160,28 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { return d.certificate, d.certificateError } -// ServeFileHTTP implements http.Handler. Returns true if something was served, false if not. +// ServeFileHTTP returns true if something was served, false if not. func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { if d == nil { httperrors.Serve404(w) return true } - if d.isCustomDomain() { - return d.serveFileFromConfig(w, r) + if !d.IsAccessControlEnabled(r) { + // Set caching headers + w.Header().Set("Cache-Control", "max-age=600") + w.Header().Set("Expires", time.Now().Add(10*time.Minute).Format(time.RFC1123)) } - return d.serveFileFromGroup(w, r) + return d.Serving.ServeFileHTTP(w, r) } -// ServeNotFoundHTTP implements http.Handler. Serves the not found pages from the projects. +// ServeNotFoundHTTP serves the not found pages from the projects. func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { if d == nil { httperrors.Serve404(w) return } - if d.isCustomDomain() { - d.serveNotFoundFromConfig(w, r) - } else { - d.serveNotFoundFromGroup(w, r) - } -} - -func endsWithSlash(path string) bool { - return strings.HasSuffix(path, "/") -} - -func endsWithoutHTMLExtension(path string) bool { - return !strings.HasSuffix(path, ".html") -} - -func openNoFollow(path string) (*os.File, error) { - return os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW, 0) + d.Serving.ServeNotFoundHTTP(w, r) } diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 552b2f15d..4ba3aba2f 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -246,29 +246,6 @@ func TestDomainCertificate(t *testing.T) { require.NoError(t, err) } -func TestOpenNoFollow(t *testing.T) { - tmpfile, err := ioutil.TempFile("", "link-test") - require.NoError(t, err) - defer tmpfile.Close() - - orig := tmpfile.Name() - softLink := orig + ".link" - defer os.Remove(orig) - - source, err := openNoFollow(orig) - require.NoError(t, err) - require.NotNil(t, source) - defer source.Close() - - err = os.Symlink(orig, softLink) - require.NoError(t, err) - defer os.Remove(softLink) - - link, err := openNoFollow(softLink) - require.Error(t, err) - require.Nil(t, link) -} - var chdirSet = false func setUpTests(t require.TestingT) func() { diff --git a/internal/serving/disk/errors.go b/internal/serving/disk/errors.go new file mode 100644 index 000000000..5e55220be --- /dev/null +++ b/internal/serving/disk/errors.go @@ -0,0 +1,18 @@ +package disk + +type locationDirectoryError struct { + FullPath string + RelativePath string +} + +type locationFileNoExtensionError struct { + FullPath string +} + +func (l *locationDirectoryError) Error() string { + return "location error accessing directory where file expected" +} + +func (l *locationFileNoExtensionError) Error() string { + return "error accessing a path without an extension" +} diff --git a/internal/serving/disk/group.go b/internal/serving/disk/group.go new file mode 100644 index 000000000..78a4ff1f8 --- /dev/null +++ b/internal/serving/disk/group.go @@ -0,0 +1,64 @@ +package disk + +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" +) + +// Group serving represents a resource that can be served from a directory +// representing GitLab group +type Group struct { + Resolver + *Reader +} + +type Resolver interface { + ProjectWithSubpath(*http.Request) (string, string, error) +} + +// ServeFileHTTP returns true if something was served, false if not. +func (g *Group) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { + return g.serveFileFromGroup(w, r) +} + +// ServeNotFoundHTTP serves the not found pages from the projects. +func (g *Group) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { + g.serveNotFoundFromGroup(w, r) +} + +func (g *Group) HasAcmeChallenge(token string) bool { + return false +} + +func (g *Group) serveFileFromGroup(w http.ResponseWriter, r *http.Request) bool { + projectName, subPath, err := g.Resolver.ProjectWithSubpath(r) + + if err != nil { + httperrors.Serve404(w) + return true + } + + if g.tryFile(w, r, projectName, subPath) == nil { + return true + } + + return false +} + +func (g *Group) serveNotFoundFromGroup(w http.ResponseWriter, r *http.Request) { + projectName, _, err := g.Resolver.ProjectWithSubpath(r) + + if err != nil { + httperrors.Serve404(w) + return + } + + // Try serving custom not-found page + if g.tryNotFound(w, r, projectName) == nil { + return + } + + // Generic 404 + httperrors.Serve404(w) +} diff --git a/internal/serving/disk/helpers.go b/internal/serving/disk/helpers.go new file mode 100644 index 000000000..7ace39be2 --- /dev/null +++ b/internal/serving/disk/helpers.go @@ -0,0 +1,77 @@ +package disk + +import ( + "io" + "mime" + "net/http" + "os" + "path/filepath" + "strings" + + "gitlab.com/gitlab-org/gitlab-pages/internal/httputil" + "golang.org/x/sys/unix" +) + +func endsWithSlash(path string) bool { + return strings.HasSuffix(path, "/") +} + +func endsWithoutHTMLExtension(path string) bool { + return !strings.HasSuffix(path, ".html") +} + +func openNoFollow(path string) (*os.File, error) { + return os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW, 0) +} + +// Detect file's content-type either by extension or mime-sniffing. +// Implementation is adapted from Golang's `http.serveContent()` +// See https://github.com/golang/go/blob/902fc114272978a40d2e65c2510a18e870077559/src/net/http/fs.go#L194 +func detectContentType(path string) (string, error) { + contentType := mime.TypeByExtension(filepath.Ext(path)) + + if contentType == "" { + var buf [512]byte + + file, err := os.Open(path) + if err != nil { + return "", err + } + + defer file.Close() + + // Using `io.ReadFull()` because `file.Read()` may be chunked. + // Ignoring errors because we don't care if the 512 bytes cannot be read. + n, _ := io.ReadFull(file, buf[:]) + contentType = http.DetectContentType(buf[:n]) + } + + return contentType, nil +} + +func acceptsGZip(r *http.Request) bool { + if r.Header.Get("Range") != "" { + return false + } + + offers := []string{"gzip", "identity"} + acceptedEncoding := httputil.NegotiateContentEncoding(r, offers) + return acceptedEncoding == "gzip" +} + +func handleGZip(w http.ResponseWriter, r *http.Request, fullPath string) string { + if !acceptsGZip(r) { + return fullPath + } + + gzipPath := fullPath + ".gz" + + // Ensure the .gz file is not a symlink + if fi, err := os.Lstat(gzipPath); err != nil || !fi.Mode().IsRegular() { + return fullPath + } + + w.Header().Set("Content-Encoding", "gzip") + + return gzipPath +} diff --git a/internal/serving/disk/helpers_test.go b/internal/serving/disk/helpers_test.go new file mode 100644 index 000000000..eea51500e --- /dev/null +++ b/internal/serving/disk/helpers_test.go @@ -0,0 +1,32 @@ +package disk + +import ( + "io/ioutil" + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestOpenNoFollow(t *testing.T) { + tmpfile, err := ioutil.TempFile("", "link-test") + require.NoError(t, err) + defer tmpfile.Close() + + orig := tmpfile.Name() + softLink := orig + ".link" + defer os.Remove(orig) + + source, err := openNoFollow(orig) + require.NoError(t, err) + require.NotNil(t, source) + defer source.Close() + + err = os.Symlink(orig, softLink) + require.NoError(t, err) + defer os.Remove(softLink) + + link, err := openNoFollow(softLink) + require.Error(t, err) + require.Nil(t, link) +} diff --git a/internal/serving/disk/project.go b/internal/serving/disk/project.go new file mode 100644 index 000000000..b2940f3d3 --- /dev/null +++ b/internal/serving/disk/project.go @@ -0,0 +1,58 @@ +package disk + +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" +) + +// Custom serving represent a resource that can be served from a directory +// representing GitLab project +type Project struct { + Location string + *Reader +} + +// ServeFileHTTP returns true if something was served, false if not. +func (p *Project) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { + return p.serveFileFromConfig(w, r) +} + +// ServeNotFoundHTTP serves the not found pages from the projects. +func (p *Project) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { + p.serveNotFoundFromConfig(w, r) +} + +func (p *Project) HasAcmeChallenge(token string) bool { + _, err := p.resolvePath(p.Location, ".well-known/acme-challenge", token) + // there is an acme challenge on disk + if err == nil { + return true + } + + _, err = p.resolvePath(p.Location, ".well-known/acme-challenge", token, "index.html") + if err == nil { + return true + } + + return false +} + +func (p *Project) serveFileFromConfig(w http.ResponseWriter, r *http.Request) bool { + // Try to serve file for http://host/... => /group/project/... + if p.tryFile(w, r, p.Location, r.URL.Path) == nil { + return true + } + + return false +} + +func (p *Project) serveNotFoundFromConfig(w http.ResponseWriter, r *http.Request) { + // Try serving not found page for http://host/ => /group/project/404.html + if p.tryNotFound(w, r, p.Location) == nil { + return + } + + // Serve generic not found + httperrors.Serve404(w) +} diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go new file mode 100644 index 000000000..068b4b47f --- /dev/null +++ b/internal/serving/disk/reader.go @@ -0,0 +1,162 @@ +package disk + +import ( + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strconv" + "strings" +) + +type Reader struct { + Group string +} + +func (reader *Reader) tryFile(w http.ResponseWriter, r *http.Request, projectName string, subPath ...string) error { + fullPath, err := reader.resolvePath(projectName, subPath...) + + if locationError, _ := err.(*locationDirectoryError); locationError != nil { + if endsWithSlash(r.URL.Path) { + fullPath, err = reader.resolvePath(projectName, filepath.Join(subPath...), "index.html") + } else { + // Concat Host with URL.Path + redirectPath := "//" + r.Host + "/" + redirectPath += strings.TrimPrefix(r.URL.Path, "/") + + // Ensure that there's always "/" at end + redirectPath = strings.TrimSuffix(redirectPath, "/") + "/" + http.Redirect(w, r, redirectPath, 302) + return nil + } + } + + if locationError, _ := err.(*locationFileNoExtensionError); locationError != nil { + fullPath, err = reader.resolvePath(projectName, strings.TrimSuffix(filepath.Join(subPath...), "/")+".html") + } + + if err != nil { + return err + } + + return reader.serveFile(w, r, fullPath) +} + +func (reader *Reader) tryNotFound(w http.ResponseWriter, r *http.Request, projectName string) error { + page404, err := reader.resolvePath(projectName, "404.html") + if err != nil { + return err + } + + err = reader.serveCustomFile(w, r, http.StatusNotFound, page404) + if err != nil { + return err + } + return nil +} + +// Resolve the HTTP request to a path on disk, converting requests for +// directories to requests for index.html inside the directory if appropriate. +func (reader *Reader) resolvePath(projectName string, subPath ...string) (string, error) { + publicPath := filepath.Join(reader.Group, projectName, "public") + + // Don't use filepath.Join as cleans the path, + // where we want to traverse full path as supplied by user + // (including ..) + testPath := publicPath + "/" + strings.Join(subPath, "/") + fullPath, err := filepath.EvalSymlinks(testPath) + if err != nil { + if endsWithoutHTMLExtension(testPath) { + return "", &locationFileNoExtensionError{ + FullPath: fullPath, + } + } + + return "", err + } + + // The requested path resolved to somewhere outside of the public/ directory + if !strings.HasPrefix(fullPath, publicPath+"/") && fullPath != publicPath { + return "", fmt.Errorf("%q should be in %q", fullPath, publicPath) + } + + fi, err := os.Lstat(fullPath) + if err != nil { + return "", err + } + + // The requested path is a directory, so try index.html via recursion + if fi.IsDir() { + return "", &locationDirectoryError{ + FullPath: fullPath, + RelativePath: strings.TrimPrefix(fullPath, publicPath), + } + } + + // The file exists, but is not a supported type to serve. Perhaps a block + // special device or something else that may be a security risk. + if !fi.Mode().IsRegular() { + return "", fmt.Errorf("%s: is not a regular file", fullPath) + } + + return fullPath, nil +} + +func (reader *Reader) serveFile(w http.ResponseWriter, r *http.Request, origPath string) error { + fullPath := handleGZip(w, r, origPath) + + file, err := openNoFollow(fullPath) + if err != nil { + return err + } + + defer file.Close() + + fi, err := file.Stat() + if err != nil { + return err + } + + contentType, err := detectContentType(origPath) + if err != nil { + return err + } + + w.Header().Set("Content-Type", contentType) + http.ServeContent(w, r, origPath, fi.ModTime(), file) + + return nil +} + +func (reader *Reader) serveCustomFile(w http.ResponseWriter, r *http.Request, code int, origPath string) error { + fullPath := handleGZip(w, r, origPath) + + // Open and serve content of file + file, err := openNoFollow(fullPath) + if err != nil { + return err + } + defer file.Close() + + fi, err := file.Stat() + if err != nil { + return err + } + + contentType, err := detectContentType(origPath) + if err != nil { + return err + } + + w.Header().Set("Content-Type", contentType) + w.Header().Set("Content-Length", strconv.FormatInt(fi.Size(), 10)) + w.WriteHeader(code) + + if r.Method != "HEAD" { + _, err := io.CopyN(w, file, fi.Size()) + return err + } + + return nil +} diff --git a/internal/serving/handler.go b/internal/serving/handler.go deleted file mode 100644 index 60b8dba90..000000000 --- a/internal/serving/handler.go +++ /dev/null @@ -1,7 +0,0 @@ -package serving - -// LegacyHandler is a struct that encapsulates legacy ResponseWriter and -// Request with all the details about a domain / address that needs to be -// served -type LegacyHandler struct { -} diff --git a/internal/serving/serving.go b/internal/serving/serving.go index bc0bb61db..0007d5827 100644 --- a/internal/serving/serving.go +++ b/internal/serving/serving.go @@ -1,9 +1,31 @@ package serving -import "net/http" +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" +) -// Serving represents an interface used to serve pages for a given domain / -// address type Serving interface { - ServeHTTP(http.ResponseWriter, *http.Request) + ServeFileHTTP(http.ResponseWriter, *http.Request) bool + ServeNotFoundHTTP(http.ResponseWriter, *http.Request) + HasAcmeChallenge(token string) bool +} + +func NewProjectDiskServing(project, group string) Serving { + return &disk.Project{ + Location: project, + Reader: &disk.Reader{ + Group: group, + }, + } +} + +func NewGroupDiskServing(group string, resolver disk.Resolver) Serving { + return &disk.Group{ + Resolver: resolver, + Reader: &disk.Reader{ + Group: group, + }, + } } diff --git a/internal/source/groups/map.go b/internal/source/groups/map.go index c72b2fe76..602e15ffa 100644 --- a/internal/source/groups/map.go +++ b/internal/source/groups/map.go @@ -13,6 +13,7 @@ import ( log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" "gitlab.com/gitlab-org/gitlab-pages/metrics" ) @@ -45,6 +46,7 @@ func (dm Map) addDomain(rootDomain, groupName, projectName string, config *Domai HTTPSOnly: config.HTTPSOnly, ProjectID: config.ID, AccessControl: config.AccessControl, + Serving: serving.NewProjectDiskServing(projectName, groupName), } var domainName string @@ -57,13 +59,16 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https groupDomain := dm[domainName] if groupDomain == nil { + group := &Group{ + name: groupName, + projects: make(projects), + subgroups: make(subgroups), + } + groupDomain = &domain.Domain{ - Group: groupName, - GroupConfig: &Group{ - name: groupName, - projects: make(projects), - subgroups: make(subgroups), - }, + Group: groupName, + GroupConfig: group, + Serving: serving.NewGroupDiskServing(groupName, group), } } -- GitLab From 6d8c6cd5402e8da0b0e118cde47f828fef740d32 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 25 Sep 2019 12:41:09 +0200 Subject: [PATCH 02/17] Generalize project config on a domain level --- internal/domain/config.go | 11 +++ internal/domain/domain.go | 52 ++++++++------ internal/domain/domain_test.go | 78 +++++++++++---------- internal/source/groups/group_domain_test.go | 29 ++++---- internal/source/groups/map.go | 20 +++--- internal/source/groups/map_test.go | 8 +-- 6 files changed, 113 insertions(+), 85 deletions(-) create mode 100644 internal/domain/config.go diff --git a/internal/domain/config.go b/internal/domain/config.go new file mode 100644 index 000000000..040b2279a --- /dev/null +++ b/internal/domain/config.go @@ -0,0 +1,11 @@ +package domain + +// ProjectConfig holds a custom project domain configuration +type ProjectConfig struct { + DomainName string + Certificate string + Key string + HTTPSOnly bool + ProjectID uint64 + AccessControl bool +} diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 9678f5fca..335543fda 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -32,8 +32,9 @@ type Domain struct { ProjectID uint64 AccessControl bool - GroupConfig GroupConfig // handles group domain config - Serving serving.Serving + GroupConfig GroupConfig // handles group domain config + ProjectConfig *ProjectConfig + Serving serving.Serving certificate *tls.Certificate certificateError error @@ -54,19 +55,31 @@ func (d *Domain) String() string { } func (d *Domain) isCustomDomain() bool { - return d.GroupConfig == nil + if d.isUnconfigured() { + panic("project config and group config should not be nil at the same time") + } + + return d.ProjectConfig != nil && d.GroupConfig == nil +} + +func (d *Domain) isUnconfigured() bool { + if d == nil { + return true + } + + return d.ProjectConfig == nil && d.GroupConfig == nil } // IsHTTPSOnly figures out if the request should be handled with HTTPS // only by looking at group and project level config. func (d *Domain) IsHTTPSOnly(r *http.Request) bool { - if d == nil { + if d.isUnconfigured() { return false } // Check custom domain config (e.g. http://example.com) if d.isCustomDomain() { - return d.HTTPSOnly + return d.ProjectConfig.HTTPSOnly } // Check projects served under the group domain, including the default one @@ -75,13 +88,13 @@ func (d *Domain) IsHTTPSOnly(r *http.Request) bool { // IsAccessControlEnabled figures out if the request is to a project that has access control enabled func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { - if d == nil { + if d.isUnconfigured() { return false } // Check custom domain config (e.g. http://example.com) if d.isCustomDomain() { - return d.AccessControl + return d.ProjectConfig.AccessControl } // Check projects served under the group domain, including the default one @@ -90,11 +103,7 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { // HasAcmeChallenge checks domain directory contains particular acme challenge func (d *Domain) HasAcmeChallenge(token string) bool { - if d == nil { - return false - } - - if !d.isCustomDomain() { + if d.isUnconfigured() || !d.isCustomDomain() { return false } @@ -103,7 +112,7 @@ func (d *Domain) HasAcmeChallenge(token string) bool { // IsNamespaceProject figures out if the request is to a namespace project func (d *Domain) IsNamespaceProject(r *http.Request) bool { - if d == nil { + if d.isUnconfigured() { return false } @@ -119,12 +128,12 @@ func (d *Domain) IsNamespaceProject(r *http.Request) bool { // GetID figures out what is the ID of the project user tries to access func (d *Domain) GetID(r *http.Request) uint64 { - if d == nil { + if d.isUnconfigured() { return 0 } if d.isCustomDomain() { - return d.ProjectID + return d.ProjectConfig.ProjectID } return d.GroupConfig.ProjectID(r) @@ -132,7 +141,7 @@ func (d *Domain) GetID(r *http.Request) uint64 { // HasProject figures out if the project exists that the user tries to access func (d *Domain) HasProject(r *http.Request) bool { - if d == nil { + if d.isUnconfigured() { return false } @@ -145,13 +154,16 @@ func (d *Domain) HasProject(r *http.Request) bool { // EnsureCertificate parses the PEM-encoded certificate for the domain func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { - if !d.isCustomDomain() { + if d.isUnconfigured() || !d.isCustomDomain() { return nil, errors.New("tls certificates can be loaded only for pages with configuration") } d.certificateOnce.Do(func() { var cert tls.Certificate - cert, d.certificateError = tls.X509KeyPair([]byte(d.Certificate), []byte(d.Key)) + cert, d.certificateError = tls.X509KeyPair( + []byte(d.ProjectConfig.Certificate), + []byte(d.ProjectConfig.Key), + ) if d.certificateError == nil { d.certificate = &cert } @@ -162,7 +174,7 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { // ServeFileHTTP returns true if something was served, false if not. func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { - if d == nil { + if d.isUnconfigured() { httperrors.Serve404(w) return true } @@ -178,7 +190,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { // ServeNotFoundHTTP serves the not found pages from the projects. func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { - if d == nil { + if d.isUnconfigured() { httperrors.Serve404(w) return } diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 4ba3aba2f..2be86e20e 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -28,9 +28,9 @@ func TestDomainServeHTTP(t *testing.T) { defer cleanup() testDomain := &Domain{ - Project: "project2", - Group: "group", - DomainName: "test.domain.com", + Project: "project2", + Group: "group", + ProjectConfig: &ProjectConfig{DomainName: "test.domain.com"}, } require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/", nil, "project2-main") @@ -54,9 +54,9 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only enabled", domain: &Domain{ - Project: "project", - Group: "group", - HTTPSOnly: true, + Project: "project", + Group: "group", + ProjectConfig: &ProjectConfig{HTTPSOnly: true}, }, url: "http://custom-domain", expected: true, @@ -64,9 +64,9 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only disabled", domain: &Domain{ - Project: "project", - Group: "group", - HTTPSOnly: false, + Project: "project", + Group: "group", + ProjectConfig: &ProjectConfig{HTTPSOnly: false}, }, url: "http://custom-domain", expected: false, @@ -103,9 +103,9 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing acme challenge", domain: &Domain{ - Group: "group.acme", - Project: "with.acme.challenge", - HTTPSOnly: true, + Group: "group.acme", + Project: "with.acme.challenge", + ProjectConfig: &ProjectConfig{HTTPSOnly: true}, }, token: "existingtoken", expected: true, @@ -113,9 +113,9 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing acme challenge", domain: &Domain{ - Group: "group.acme", - Project: "with.acme.challenge", - HTTPSOnly: true, + Group: "group.acme", + Project: "with.acme.challenge", + ProjectConfig: &ProjectConfig{HTTPSOnly: true}, }, token: "foldertoken", expected: true, @@ -123,9 +123,9 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing another token", domain: &Domain{ - Group: "group.acme", - Project: "with.acme.challenge", - HTTPSOnly: true, + Group: "group.acme", + Project: "with.acme.challenge", + ProjectConfig: &ProjectConfig{HTTPSOnly: true}, }, token: "notexistingtoken", expected: false, @@ -136,15 +136,15 @@ func TestHasAcmeChallenge(t *testing.T) { token: "existingtoken", expected: false, }, - // { TODO ask someone why this tests needs to be passing - // name: "Domain without config", - // domain: &Domain{ - // Group: "group.acme", - // Project: "with.acme.challenge", - // }, - // token: "existingtoken", - // expected: false, - // }, + { + name: "Domain without config", + domain: &Domain{ + Group: "group.acme", + Project: "with.acme.challenge", + }, + token: "existingtoken", + expected: false, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -185,9 +185,9 @@ func TestDomain404ServeHTTP(t *testing.T) { defer cleanup() testDomain := &Domain{ - Group: "group.404", - Project: "domain.404", - DomainName: "domain.404.com", + Group: "group.404", + Project: "domain.404", + ProjectConfig: &ProjectConfig{DomainName: "domain.404.com"}, } testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") @@ -218,9 +218,9 @@ func TestGroupCertificate(t *testing.T) { func TestDomainNoCertificate(t *testing.T) { testDomain := &Domain{ - Group: "group", - Project: "project2", - DomainName: "test.domain.com", + Group: "group", + Project: "project2", + ProjectConfig: &ProjectConfig{DomainName: "test.domain.com"}, } tls, err := testDomain.EnsureCertificate() @@ -234,11 +234,13 @@ func TestDomainNoCertificate(t *testing.T) { func TestDomainCertificate(t *testing.T) { testDomain := &Domain{ - Group: "group", - Project: "project2", - DomainName: "test.domain.com", - Certificate: fixture.Certificate, - Key: fixture.Key, + Group: "group", + Project: "project2", + ProjectConfig: &ProjectConfig{ + DomainName: "test.domain.com", + Certificate: fixture.Certificate, + Key: fixture.Key, + }, } tls, err := testDomain.EnsureCertificate() diff --git a/internal/source/groups/group_domain_test.go b/internal/source/groups/group_domain_test.go index 38225cf51..b48aa771b 100644 --- a/internal/source/groups/group_domain_test.go +++ b/internal/source/groups/group_domain_test.go @@ -82,9 +82,9 @@ func TestDomainServeHTTP(t *testing.T) { defer cleanup() testDomain := &domain.Domain{ - Group: "group", - Project: "project2", - DomainName: "test.domain.com", + Group: "group", + Project: "project2", + ProjectConfig: &domain.ProjectConfig{DomainName: "test.domain.com"}, } require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/", nil, "project2-main") @@ -319,9 +319,9 @@ func TestDomain404ServeHTTP(t *testing.T) { defer cleanup() testDomain := &domain.Domain{ - Project: "domain.404", - Group: "group.404", - DomainName: "domain.404.com", + Project: "domain.404", + Group: "group.404", + ProjectConfig: &domain.ProjectConfig{DomainName: "domain.404.com"}, } testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") @@ -352,9 +352,9 @@ func TestGroupCertificate(t *testing.T) { func TestDomainNoCertificate(t *testing.T) { testDomain := &domain.Domain{ - Group: "group", - Project: "project2", - DomainName: "test.domain.com", + Group: "group", + Project: "project2", + ProjectConfig: &domain.ProjectConfig{DomainName: "test.domain.com"}, } tls, err := testDomain.EnsureCertificate() @@ -368,11 +368,12 @@ func TestDomainNoCertificate(t *testing.T) { func TestDomainCertificate(t *testing.T) { testDomain := &domain.Domain{ - Project: "project2", - Group: "group", - DomainName: "test.domain.com", - Certificate: fixture.Certificate, - Key: fixture.Key, + Project: "project2", + Group: "group", + ProjectConfig: &domain.ProjectConfig{DomainName: "test.domain.com", + Certificate: fixture.Certificate, + Key: fixture.Key, + }, } tls, err := testDomain.EnsureCertificate() diff --git a/internal/source/groups/map.go b/internal/source/groups/map.go index 602e15ffa..670c9bb86 100644 --- a/internal/source/groups/map.go +++ b/internal/source/groups/map.go @@ -38,15 +38,17 @@ func (dm Map) updateDomainMap(domainName string, domain *domain.Domain) { func (dm Map) addDomain(rootDomain, groupName, projectName string, config *DomainConfig) { newDomain := &domain.Domain{ - Group: groupName, - Project: projectName, - DomainName: config.Domain, - Certificate: config.Certificate, - Key: config.Key, - HTTPSOnly: config.HTTPSOnly, - ProjectID: config.ID, - AccessControl: config.AccessControl, - Serving: serving.NewProjectDiskServing(projectName, groupName), + Group: groupName, + Project: projectName, + ProjectConfig: &domain.ProjectConfig{ + DomainName: config.Domain, + Certificate: config.Certificate, + Key: config.Key, + HTTPSOnly: config.HTTPSOnly, + ProjectID: config.ID, + AccessControl: config.AccessControl, + }, + Serving: serving.NewProjectDiskServing(projectName, groupName), } var domainName string diff --git a/internal/source/groups/map_test.go b/internal/source/groups/map_test.go index 4f333c9ae..2acb9f20a 100644 --- a/internal/source/groups/map_test.go +++ b/internal/source/groups/map_test.go @@ -68,10 +68,10 @@ func TestReadProjects(t *testing.T) { } // Check that multiple domains in the same project are recorded faithfully - require.Equal(t, "test.domain.com", dm["test.domain.com"].DomainName) - require.Equal(t, "other.domain.com", dm["other.domain.com"].DomainName) - require.Equal(t, "test", dm["other.domain.com"].Certificate) - require.Equal(t, "key", dm["other.domain.com"].Key) + require.Equal(t, "test.domain.com", dm["test.domain.com"].ProjectConfig.DomainName) + require.Equal(t, "other.domain.com", dm["other.domain.com"].ProjectConfig.DomainName) + require.Equal(t, "test", dm["other.domain.com"].ProjectConfig.Certificate) + require.Equal(t, "key", dm["other.domain.com"].ProjectConfig.Key) // check subgroups domain, ok := dm["group.test.io"] -- GitLab From 88a822a89ce35aeb94883799a9585e37be4dd1ab Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 25 Sep 2019 12:44:30 +0200 Subject: [PATCH 03/17] Rename groups package to disk package --- internal/source/{groups => disk}/config.go | 2 +- internal/source/{groups => disk}/config_test.go | 2 +- internal/source/{groups => disk}/group.go | 2 +- internal/source/{groups => disk}/group_domain_test.go | 2 +- internal/source/{groups => disk}/group_test.go | 2 +- internal/source/{groups => disk}/map.go | 2 +- internal/source/{groups => disk}/map_test.go | 2 +- internal/source/domains.go | 8 ++++---- 8 files changed, 11 insertions(+), 11 deletions(-) rename internal/source/{groups => disk}/config.go (98%) rename internal/source/{groups => disk}/config_test.go (99%) rename internal/source/{groups => disk}/group.go (99%) rename internal/source/{groups => disk}/group_domain_test.go (99%) rename internal/source/{groups => disk}/group_test.go (99%) rename internal/source/{groups => disk}/map.go (99%) rename internal/source/{groups => disk}/map_test.go (99%) diff --git a/internal/source/groups/config.go b/internal/source/disk/config.go similarity index 98% rename from internal/source/groups/config.go rename to internal/source/disk/config.go index 889472598..de3576d22 100644 --- a/internal/source/groups/config.go +++ b/internal/source/disk/config.go @@ -1,4 +1,4 @@ -package groups +package disk import ( "encoding/json" diff --git a/internal/source/groups/config_test.go b/internal/source/disk/config_test.go similarity index 99% rename from internal/source/groups/config_test.go rename to internal/source/disk/config_test.go index ed563b87f..c0e22c04f 100644 --- a/internal/source/groups/config_test.go +++ b/internal/source/disk/config_test.go @@ -1,4 +1,4 @@ -package groups +package disk import ( "io/ioutil" diff --git a/internal/source/groups/group.go b/internal/source/disk/group.go similarity index 99% rename from internal/source/groups/group.go rename to internal/source/disk/group.go index e5534cf69..fd96124d9 100644 --- a/internal/source/groups/group.go +++ b/internal/source/disk/group.go @@ -1,4 +1,4 @@ -package groups +package disk import ( "errors" diff --git a/internal/source/groups/group_domain_test.go b/internal/source/disk/group_domain_test.go similarity index 99% rename from internal/source/groups/group_domain_test.go rename to internal/source/disk/group_domain_test.go index b48aa771b..8fcb0f1b6 100644 --- a/internal/source/groups/group_domain_test.go +++ b/internal/source/disk/group_domain_test.go @@ -1,4 +1,4 @@ -package groups +package disk import ( "compress/gzip" diff --git a/internal/source/groups/group_test.go b/internal/source/disk/group_test.go similarity index 99% rename from internal/source/groups/group_test.go rename to internal/source/disk/group_test.go index a8070c87c..2aa3fc4eb 100644 --- a/internal/source/groups/group_test.go +++ b/internal/source/disk/group_test.go @@ -1,4 +1,4 @@ -package groups +package disk import ( "strings" diff --git a/internal/source/groups/map.go b/internal/source/disk/map.go similarity index 99% rename from internal/source/groups/map.go rename to internal/source/disk/map.go index 670c9bb86..66c0a72c2 100644 --- a/internal/source/groups/map.go +++ b/internal/source/disk/map.go @@ -1,4 +1,4 @@ -package groups +package disk import ( "bytes" diff --git a/internal/source/groups/map_test.go b/internal/source/disk/map_test.go similarity index 99% rename from internal/source/groups/map_test.go rename to internal/source/disk/map_test.go index 2acb9f20a..9aa960722 100644 --- a/internal/source/groups/map_test.go +++ b/internal/source/disk/map_test.go @@ -1,4 +1,4 @@ -package groups +package disk import ( "crypto/rand" diff --git a/internal/source/domains.go b/internal/source/domains.go index 5e7f113cf..54a269d86 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -6,13 +6,13 @@ import ( "time" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/groups" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" ) // Domains struct represents a map of all domains supported by pages. It is // currently reading them from disk. type Domains struct { - dm groups.Map + dm disk.Map lock sync.RWMutex } @@ -45,10 +45,10 @@ func (d *Domains) Ready() bool { // Watch starts the domain source, in this case it is reading domains from // groups on disk concurrently func (d *Domains) Watch(rootDomain string) { - go groups.Watch(rootDomain, d.updateDomains, time.Second) + go disk.Watch(rootDomain, d.updateDomains, time.Second) } -func (d *Domains) updateDomains(dm groups.Map) { +func (d *Domains) updateDomains(dm disk.Map) { d.lock.Lock() defer d.lock.Unlock() -- GitLab From 1094a225e69c8b139196aa55399bfb8d8c4e47fd Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 25 Sep 2019 12:52:55 +0200 Subject: [PATCH 04/17] Do not export disk source internal config types --- internal/source/disk/config.go | 12 +++--- internal/source/disk/config_test.go | 26 ++++++------- internal/source/disk/group.go | 6 +-- internal/source/disk/group_domain_test.go | 46 +++++++++++------------ internal/source/disk/group_test.go | 10 ++--- internal/source/disk/map.go | 10 ++--- 6 files changed, 55 insertions(+), 55 deletions(-) diff --git a/internal/source/disk/config.go b/internal/source/disk/config.go index de3576d22..d2e6c123b 100644 --- a/internal/source/disk/config.go +++ b/internal/source/disk/config.go @@ -8,7 +8,7 @@ import ( ) // DomainConfig represents a custom domain config -type DomainConfig struct { +type domainConfig struct { Domain string Certificate string Key string @@ -18,15 +18,15 @@ type DomainConfig struct { } // MultiDomainConfig represents a group of custom domain configs -type MultiDomainConfig struct { - Domains []DomainConfig +type multiDomainConfig struct { + Domains []domainConfig HTTPSOnly bool `json:"https_only"` ID uint64 `json:"id"` AccessControl bool `json:"access_control"` } // ProjectConfig is a project-level configuration -type ProjectConfig struct { +type projectConfig struct { NamespaceProject bool HTTPSOnly bool AccessControl bool @@ -34,7 +34,7 @@ type ProjectConfig struct { } // Valid validates a custom domain config for a root domain -func (c *DomainConfig) Valid(rootDomain string) bool { +func (c *domainConfig) Valid(rootDomain string) bool { if c.Domain == "" { return false } @@ -46,7 +46,7 @@ func (c *DomainConfig) Valid(rootDomain string) bool { } // Read reads a multi domain config and decodes it from a `config.json` -func (c *MultiDomainConfig) Read(group, project string) error { +func (c *multiDomainConfig) Read(group, project string) error { configFile, err := os.Open(filepath.Join(group, project, "config.json")) if err != nil { return err diff --git a/internal/source/disk/config_test.go b/internal/source/disk/config_test.go index c0e22c04f..ec2bb8548 100644 --- a/internal/source/disk/config_test.go +++ b/internal/source/disk/config_test.go @@ -13,53 +13,53 @@ const configFile = "test-group/test-project/config.json" const invalidConfig = `{"Domains":{}}` const validConfig = `{"Domains":[{"Domain":"test"}]}` -func TestDomainConfigValidness(t *testing.T) { - d := DomainConfig{} +func TestdomainConfigValidness(t *testing.T) { + d := domainConfig{} require.False(t, d.Valid("gitlab.io")) - d = DomainConfig{Domain: "test"} + d = domainConfig{Domain: "test"} require.True(t, d.Valid("gitlab.io")) - d = DomainConfig{Domain: "test"} + d = domainConfig{Domain: "test"} require.True(t, d.Valid("gitlab.io")) - d = DomainConfig{Domain: "test.gitlab.io"} + d = domainConfig{Domain: "test.gitlab.io"} require.False(t, d.Valid("gitlab.io")) - d = DomainConfig{Domain: "test.test.gitlab.io"} + d = domainConfig{Domain: "test.test.gitlab.io"} require.False(t, d.Valid("gitlab.io")) - d = DomainConfig{Domain: "test.testgitlab.io"} + d = domainConfig{Domain: "test.testgitlab.io"} require.True(t, d.Valid("gitlab.io")) - d = DomainConfig{Domain: "test.GitLab.Io"} + d = domainConfig{Domain: "test.GitLab.Io"} require.False(t, d.Valid("gitlab.io")) } -func TestDomainConfigRead(t *testing.T) { +func TestdomainConfigRead(t *testing.T) { cleanup := setUpTests(t) defer cleanup() - d := MultiDomainConfig{} + d := multiDomainConfig{} err := d.Read("test-group", "test-project") require.Error(t, err) os.MkdirAll(filepath.Dir(configFile), 0700) defer os.RemoveAll("test-group") - d = MultiDomainConfig{} + d = multiDomainConfig{} err = d.Read("test-group", "test-project") require.Error(t, err) err = ioutil.WriteFile(configFile, []byte(invalidConfig), 0600) require.NoError(t, err) - d = MultiDomainConfig{} + d = multiDomainConfig{} err = d.Read("test-group", "test-project") require.Error(t, err) err = ioutil.WriteFile(configFile, []byte(validConfig), 0600) require.NoError(t, err) - d = MultiDomainConfig{} + d = multiDomainConfig{} err = d.Read("test-group", "test-project") require.NoError(t, err) } diff --git a/internal/source/disk/group.go b/internal/source/disk/group.go index fd96124d9..0c8d08104 100644 --- a/internal/source/disk/group.go +++ b/internal/source/disk/group.go @@ -28,10 +28,10 @@ type Group struct { projects projects } -type projects map[string]*ProjectConfig +type projects map[string]*projectConfig type subgroups map[string]*Group -func (g *Group) digProjectWithSubpath(parentPath string, keys []string) (*ProjectConfig, string, string) { +func (g *Group) digProjectWithSubpath(parentPath string, keys []string) (*projectConfig, string, string) { if len(keys) >= 1 { head := keys[0] tail := keys[1:] @@ -52,7 +52,7 @@ func (g *Group) digProjectWithSubpath(parentPath string, keys []string) (*Projec // Look up a project inside the domain based on the host and path. Returns the // project and its name (if applicable) -func (g *Group) getProjectConfigWithSubpath(r *http.Request) (*ProjectConfig, string, string) { +func (g *Group) getProjectConfigWithSubpath(r *http.Request) (*projectConfig, string, string) { // Check for a project specified in the URL: http://group.gitlab.io/projectA // If present, these projects shadow the group domain. split := strings.SplitN(r.URL.Path, "/", maxProjectDepth) diff --git a/internal/source/disk/group_domain_test.go b/internal/source/disk/group_domain_test.go index 8fcb0f1b6..3b4471f4f 100644 --- a/internal/source/disk/group_domain_test.go +++ b/internal/source/disk/group_domain_test.go @@ -31,11 +31,11 @@ func testGroupServeHTTPHost(t *testing.T, host string) { Group: "group", GroupConfig: &Group{ name: "group", - projects: map[string]*ProjectConfig{ - "group.test.io": &ProjectConfig{}, - "group.gitlab-example.com": &ProjectConfig{}, - "project": &ProjectConfig{}, - "project2": &ProjectConfig{}, + projects: map[string]*projectConfig{ + "group.test.io": &projectConfig{}, + "group.gitlab-example.com": &projectConfig{}, + "project": &projectConfig{}, + "project2": &projectConfig{}, }, }, } @@ -112,7 +112,7 @@ func TestIsHTTPSOnly(t *testing.T) { Project: "project", GroupConfig: &Group{ name: "group", - projects: projects{"test-domain": &ProjectConfig{HTTPSOnly: true}}, + projects: projects{"test-domain": &projectConfig{HTTPSOnly: true}}, }, }, url: "http://test-domain", @@ -125,7 +125,7 @@ func TestIsHTTPSOnly(t *testing.T) { Project: "project", GroupConfig: &Group{ name: "group", - projects: projects{"test-domain": &ProjectConfig{HTTPSOnly: false}}, + projects: projects{"test-domain": &projectConfig{HTTPSOnly: false}}, }, }, url: "http://test-domain", @@ -138,7 +138,7 @@ func TestIsHTTPSOnly(t *testing.T) { Group: "group", GroupConfig: &Group{ name: "group", - projects: projects{"test-domain": &ProjectConfig{HTTPSOnly: true}}, + projects: projects{"test-domain": &projectConfig{HTTPSOnly: true}}, }, }, url: "http://Test-domain", @@ -151,7 +151,7 @@ func TestIsHTTPSOnly(t *testing.T) { Group: "group", GroupConfig: &Group{ name: "group", - projects: projects{"project": &ProjectConfig{HTTPSOnly: true}}, + projects: projects{"project": &projectConfig{HTTPSOnly: true}}, }, }, url: "http://test-domain/project", @@ -164,7 +164,7 @@ func TestIsHTTPSOnly(t *testing.T) { Group: "group", GroupConfig: &Group{ name: "group", - projects: projects{"project": &ProjectConfig{HTTPSOnly: false}}, + projects: projects{"project": &projectConfig{HTTPSOnly: false}}, }, }, url: "http://test-domain/project", @@ -225,11 +225,11 @@ func TestGroupServeHTTPGzip(t *testing.T) { Group: "group", GroupConfig: &Group{ name: "group", - projects: map[string]*ProjectConfig{ - "group.test.io": &ProjectConfig{}, - "group.gitlab-example.com": &ProjectConfig{}, - "project": &ProjectConfig{}, - "project2": &ProjectConfig{}, + projects: map[string]*projectConfig{ + "group.test.io": &projectConfig{}, + "group.gitlab-example.com": &projectConfig{}, + "project": &projectConfig{}, + "project2": &projectConfig{}, }, }, } @@ -293,12 +293,12 @@ func TestGroup404ServeHTTP(t *testing.T) { Group: "group.404", GroupConfig: &Group{ name: "group.404", - projects: map[string]*ProjectConfig{ - "domain.404": &ProjectConfig{}, - "group.404.test.io": &ProjectConfig{}, - "project.404": &ProjectConfig{}, - "project.404.symlink": &ProjectConfig{}, - "project.no.404": &ProjectConfig{}, + projects: map[string]*projectConfig{ + "domain.404": &projectConfig{}, + "group.404.test.io": &projectConfig{}, + "project.404": &projectConfig{}, + "project.404.symlink": &projectConfig{}, + "project.no.404": &projectConfig{}, }, }, } @@ -389,8 +389,8 @@ func TestCacheControlHeaders(t *testing.T) { Group: "group", GroupConfig: &Group{ name: "group", - projects: map[string]*ProjectConfig{ - "group.test.io": &ProjectConfig{}, + projects: map[string]*projectConfig{ + "group.test.io": &projectConfig{}, }, }, } diff --git a/internal/source/disk/group_test.go b/internal/source/disk/group_test.go index 2aa3fc4eb..d0fb49bd9 100644 --- a/internal/source/disk/group_test.go +++ b/internal/source/disk/group_test.go @@ -8,13 +8,13 @@ import ( ) func TestGroupDig(t *testing.T) { - matchingProject := &ProjectConfig{ID: 1} + matchingProject := &projectConfig{ID: 1} tests := []struct { name string g Group path string - expectedProject *ProjectConfig + expectedProject *projectConfig expectedProjectPath string expectedPath string }{ @@ -49,7 +49,7 @@ func TestGroupDig(t *testing.T) { projects: projects{"projectb": matchingProject}, subgroups: subgroups{ "sub1": &Group{ - projects: projects{"another": &ProjectConfig{}}, + projects: projects{"another": &projectConfig{}}, }, }, }, @@ -66,7 +66,7 @@ func TestGroupDig(t *testing.T) { projects: projects{"projectb": matchingProject}, }, }, - projects: projects{"another": &ProjectConfig{}}, + projects: projects{"another": &projectConfig{}}, }, expectedProject: matchingProject, expectedProjectPath: "sub1/projectb", @@ -78,7 +78,7 @@ func TestGroupDig(t *testing.T) { g: Group{ subgroups: subgroups{ "sub1": &Group{ - projects: projects{"another": &ProjectConfig{}}, + projects: projects{"another": &projectConfig{}}, }, }, }, diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index 66c0a72c2..39d170a76 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -36,7 +36,7 @@ func (dm Map) updateDomainMap(domainName string, domain *domain.Domain) { dm[domainName] = domain } -func (dm Map) addDomain(rootDomain, groupName, projectName string, config *DomainConfig) { +func (dm Map) addDomain(rootDomain, groupName, projectName string, config *domainConfig) { newDomain := &domain.Domain{ Group: groupName, Project: projectName, @@ -93,7 +93,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https g = subgroup } - g.projects[projectName] = &ProjectConfig{ + g.projects[projectName] = &projectConfig{ NamespaceProject: domainName == projectName, HTTPSOnly: httpsOnly, AccessControl: accessControl, @@ -103,7 +103,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https dm[domainName] = groupDomain } -func (dm Map) readProjectConfig(rootDomain string, group, projectName string, config *MultiDomainConfig) { +func (dm Map) readProjectConfig(rootDomain string, group, projectName string, config *multiDomainConfig) { if config == nil { // This is necessary to preserve the previous behaviour where a // group domain is created even if no config.json files are @@ -145,7 +145,7 @@ func readProject(group, parent, projectName string, level int, fanIn chan<- jobR // We read the config.json file _before_ fanning in, because it does disk // IO and it does not need access to the domains map. - config := &MultiDomainConfig{} + config := &multiDomainConfig{} if err := config.Read(group, projectPath); err != nil { config = nil } @@ -177,7 +177,7 @@ func readProjects(group, parent string, level int, buf []byte, fanIn chan<- jobR type jobResult struct { group string project string - config *MultiDomainConfig + config *multiDomainConfig } // ReadGroups walks the pages directory and populates dm with all the domains it finds. -- GitLab From 05d15b9252a71d52570b180b6360bbb61238d83f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 26 Sep 2019 11:00:37 +0200 Subject: [PATCH 05/17] Remove unused domain settings fields --- internal/domain/domain.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 335543fda..79f9abf6f 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -25,13 +25,6 @@ type Domain struct { Group string Project string - DomainName string - Certificate string - Key string - HTTPSOnly bool - ProjectID uint64 - AccessControl bool - GroupConfig GroupConfig // handles group domain config ProjectConfig *ProjectConfig Serving serving.Serving -- GitLab From bf3b3983eae2cb83e82224d4f936ccee1b4322ef Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 26 Sep 2019 11:08:01 +0200 Subject: [PATCH 06/17] Initialize domain serving in a Domain type --- internal/domain/domain.go | 23 +++++++++++++++++++---- internal/serving/serving.go | 1 + internal/source/disk/map.go | 3 --- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 79f9abf6f..361c3b873 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -18,6 +18,7 @@ type GroupConfig interface { IsNamespaceProject(*http.Request) bool ProjectID(*http.Request) uint64 ProjectExists(*http.Request) bool + ProjectWithSubpath(*http.Request) (string, string, error) } // Domain is a domain that gitlab-pages can serve. @@ -27,7 +28,8 @@ type Domain struct { GroupConfig GroupConfig // handles group domain config ProjectConfig *ProjectConfig - Serving serving.Serving + + serving serving.Serving certificate *tls.Certificate certificateError error @@ -63,6 +65,19 @@ func (d *Domain) isUnconfigured() bool { return d.ProjectConfig == nil && d.GroupConfig == nil } +// Serving returns domain serving driver +func (d *Domain) Serving() serving.Serving { + if d.serving == nil { + if d.isCustomDomain() { + d.serving = serving.NewProjectDiskServing(d.Project, d.Group) + } else { + d.serving = serving.NewGroupDiskServing(d.Group, d.GroupConfig) + } + } + + return d.serving +} + // IsHTTPSOnly figures out if the request should be handled with HTTPS // only by looking at group and project level config. func (d *Domain) IsHTTPSOnly(r *http.Request) bool { @@ -100,7 +115,7 @@ func (d *Domain) HasAcmeChallenge(token string) bool { return false } - return d.Serving.HasAcmeChallenge(token) + return d.Serving().HasAcmeChallenge(token) } // IsNamespaceProject figures out if the request is to a namespace project @@ -178,7 +193,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { w.Header().Set("Expires", time.Now().Add(10*time.Minute).Format(time.RFC1123)) } - return d.Serving.ServeFileHTTP(w, r) + return d.Serving().ServeFileHTTP(w, r) } // ServeNotFoundHTTP serves the not found pages from the projects. @@ -188,5 +203,5 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { return } - d.Serving.ServeNotFoundHTTP(w, r) + d.Serving().ServeNotFoundHTTP(w, r) } diff --git a/internal/serving/serving.go b/internal/serving/serving.go index 0007d5827..b28d09001 100644 --- a/internal/serving/serving.go +++ b/internal/serving/serving.go @@ -6,6 +6,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" ) +// Serving is an interface used to define a serving driver type Serving interface { ServeFileHTTP(http.ResponseWriter, *http.Request) bool ServeNotFoundHTTP(http.ResponseWriter, *http.Request) diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index 39d170a76..dd368501c 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -13,7 +13,6 @@ import ( log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" - "gitlab.com/gitlab-org/gitlab-pages/internal/serving" "gitlab.com/gitlab-org/gitlab-pages/metrics" ) @@ -48,7 +47,6 @@ func (dm Map) addDomain(rootDomain, groupName, projectName string, config *domai ProjectID: config.ID, AccessControl: config.AccessControl, }, - Serving: serving.NewProjectDiskServing(projectName, groupName), } var domainName string @@ -70,7 +68,6 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https groupDomain = &domain.Domain{ Group: groupName, GroupConfig: group, - Serving: serving.NewGroupDiskServing(groupName, group), } } -- GitLab From 3cc7bc556307adbc8fac6942240bafceb0ff5d5d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 26 Sep 2019 14:14:51 +0200 Subject: [PATCH 07/17] Unify how we handle custom and group domain in serving --- go.mod | 2 + internal/acme/acme.go | 4 +- internal/acme/acme_test.go | 2 +- internal/domain/config.go | 11 -- internal/domain/domain.go | 138 ++++++++-------- internal/domain/domain_test.go | 148 +++--------------- internal/domain/handler.go | 42 +++++ internal/domain/project.go | 10 ++ internal/domain/resolver.go | 10 ++ internal/serving/disk.go | 19 +++ internal/serving/disk/group.go | 64 -------- internal/serving/disk/handler.go | 11 ++ internal/serving/disk/project.go | 58 ------- internal/serving/disk/reader.go | 44 ++++-- internal/serving/disk/serving.go | 47 ++++++ internal/serving/handler.go | 16 ++ internal/serving/serving.go | 29 ++-- internal/source/disk/custom.go | 24 +++ .../{group_domain_test.go => domain_test.go} | 86 +++++----- internal/source/disk/group.go | 78 ++------- internal/source/disk/map.go | 38 ++--- internal/source/disk/map_test.go | 12 +- 22 files changed, 390 insertions(+), 503 deletions(-) delete mode 100644 internal/domain/config.go create mode 100644 internal/domain/handler.go create mode 100644 internal/domain/project.go create mode 100644 internal/domain/resolver.go create mode 100644 internal/serving/disk.go delete mode 100644 internal/serving/disk/group.go create mode 100644 internal/serving/disk/handler.go delete mode 100644 internal/serving/disk/project.go create mode 100644 internal/serving/disk/serving.go create mode 100644 internal/serving/handler.go create mode 100644 internal/source/disk/custom.go rename internal/source/disk/{group_domain_test.go => domain_test.go} (92%) diff --git a/go.mod b/go.mod index 2c4252a45..046398e28 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,8 @@ require ( github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 github.com/karrick/godirwalk v1.10.12 + github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect + github.com/modern-go/reflect2 v1.0.1 // indirect github.com/namsral/flag v1.7.4-pre github.com/prometheus/client_golang v1.1.0 github.com/rs/cors v1.7.0 diff --git a/internal/acme/acme.go b/internal/acme/acme.go index 89881f34b..a6be01dec 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -18,7 +18,7 @@ type Middleware struct { // Domain interface represent D from domain package type Domain interface { - HasAcmeChallenge(string) bool + HasAcmeChallenge(*http.Request, string) bool } // ServeAcmeChallenges identifies if request is acme-challenge and redirects to GitLab in that case @@ -31,7 +31,7 @@ func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, return false } - if domain.HasAcmeChallenge(filepath.Base(r.URL.Path)) { + if domain.HasAcmeChallenge(r, filepath.Base(r.URL.Path)) { return false } diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go index c0daefebe..00932d3e9 100644 --- a/internal/acme/acme_test.go +++ b/internal/acme/acme_test.go @@ -11,7 +11,7 @@ type domainStub struct { hasAcmeChallenge bool } -func (d *domainStub) HasAcmeChallenge(_ string) bool { +func (d *domainStub) HasAcmeChallenge(_ *http.Request, _ string) bool { return d.hasAcmeChallenge } diff --git a/internal/domain/config.go b/internal/domain/config.go deleted file mode 100644 index 040b2279a..000000000 --- a/internal/domain/config.go +++ /dev/null @@ -1,11 +0,0 @@ -package domain - -// ProjectConfig holds a custom project domain configuration -type ProjectConfig struct { - DomainName string - Certificate string - Key string - HTTPSOnly bool - ProjectID uint64 - AccessControl bool -} diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 361c3b873..1dd5be51e 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -5,31 +5,23 @@ import ( "errors" "net/http" "sync" - "time" "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" ) -// GroupConfig represents a per-request config for a group domain -type GroupConfig interface { - IsHTTPSOnly(*http.Request) bool - HasAccessControl(*http.Request) bool - IsNamespaceProject(*http.Request) bool - ProjectID(*http.Request) uint64 - ProjectExists(*http.Request) bool - ProjectWithSubpath(*http.Request) (string, string, error) -} - // Domain is a domain that gitlab-pages can serve. type Domain struct { - Group string - Project string + Name string + Location string + CertificateCert string + CertificateKey string + Customized bool // TODO we should get rid of this - GroupConfig GroupConfig // handles group domain config - ProjectConfig *ProjectConfig + Resolver Resolver - serving serving.Serving + lookupPaths map[string]*Project + serving serving.Serving certificate *tls.Certificate certificateError error @@ -38,15 +30,7 @@ type Domain struct { // String implements Stringer. func (d *Domain) String() string { - if d.Group != "" && d.Project != "" { - return d.Group + "/" + d.Project - } - - if d.Group != "" { - return d.Group - } - - return d.Project + return d.Name } func (d *Domain) isCustomDomain() bool { @@ -54,7 +38,7 @@ func (d *Domain) isCustomDomain() bool { panic("project config and group config should not be nil at the same time") } - return d.ProjectConfig != nil && d.GroupConfig == nil + return d.Customized } func (d *Domain) isUnconfigured() bool { @@ -62,22 +46,52 @@ func (d *Domain) isUnconfigured() bool { return true } - return d.ProjectConfig == nil && d.GroupConfig == nil + return d.Resolver == nil +} + +func (d *Domain) resolve(r *http.Request) (*Project, string) { + // TODO use lookupPaths first + + project, subpath, _ := d.Resolver.Resolve(r) + // current implementation does not return errors in any case + + if project == nil { + return nil, "" + } + + return project, subpath +} + +func (d *Domain) getProject(r *http.Request) *Project { + project, _ := d.resolve(r) + + return project } // Serving returns domain serving driver func (d *Domain) Serving() serving.Serving { if d.serving == nil { - if d.isCustomDomain() { - d.serving = serving.NewProjectDiskServing(d.Project, d.Group) - } else { - d.serving = serving.NewGroupDiskServing(d.Group, d.GroupConfig) - } + d.serving = serving.NewDiskServing(d.Name, d.Location) } return d.serving } +func (d *Domain) toHandler(w http.ResponseWriter, r *http.Request) *handler { + project, subpath := d.resolve(r) + + return &handler{ + writer: w, + request: r, + project: project, + subpath: subpath, + } +} + +func (d *Domain) hasProject(r *http.Request) bool { + return d.getProject(r) != nil +} + // IsHTTPSOnly figures out if the request should be handled with HTTPS // only by looking at group and project level config. func (d *Domain) IsHTTPSOnly(r *http.Request) bool { @@ -85,13 +99,11 @@ func (d *Domain) IsHTTPSOnly(r *http.Request) bool { return false } - // Check custom domain config (e.g. http://example.com) - if d.isCustomDomain() { - return d.ProjectConfig.HTTPSOnly + if project := d.getProject(r); project != nil { + return project.IsHTTPSOnly } - // Check projects served under the group domain, including the default one - return d.GroupConfig.IsHTTPSOnly(r) + return false } // IsAccessControlEnabled figures out if the request is to a project that has access control enabled @@ -100,22 +112,20 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { return false } - // Check custom domain config (e.g. http://example.com) - if d.isCustomDomain() { - return d.ProjectConfig.AccessControl + if project := d.getProject(r); project != nil { + return project.HasAccessControl } - // Check projects served under the group domain, including the default one - return d.GroupConfig.HasAccessControl(r) + return false } // HasAcmeChallenge checks domain directory contains particular acme challenge -func (d *Domain) HasAcmeChallenge(token string) bool { - if d.isUnconfigured() || !d.isCustomDomain() { +func (d *Domain) HasAcmeChallenge(r *http.Request, token string) bool { + if d.isUnconfigured() || !d.isCustomDomain() || !d.hasProject(r) { return false } - return d.Serving().HasAcmeChallenge(token) + return d.Serving().HasAcmeChallenge(d.toHandler(nil, r), token) // TODO } // IsNamespaceProject figures out if the request is to a namespace project @@ -126,12 +136,15 @@ func (d *Domain) IsNamespaceProject(r *http.Request) bool { // If request is to a custom domain, we do not handle it as a namespace project // as there can't be multiple projects under the same custom domain - if d.isCustomDomain() { + if d.isCustomDomain() { // TODO do we need a separate path for this return false } - // Check projects served under the group domain, including the default one - return d.GroupConfig.IsNamespaceProject(r) + if project := d.getProject(r); project != nil { + return project.IsNamespaceProject + } + + return false } // GetID figures out what is the ID of the project user tries to access @@ -140,11 +153,11 @@ func (d *Domain) GetID(r *http.Request) uint64 { return 0 } - if d.isCustomDomain() { - return d.ProjectConfig.ProjectID + if project := d.getProject(r); project != nil { + return project.ID } - return d.GroupConfig.ProjectID(r) + return 0 } // HasProject figures out if the project exists that the user tries to access @@ -153,15 +166,16 @@ func (d *Domain) HasProject(r *http.Request) bool { return false } - if d.isCustomDomain() { + if project := d.getProject(r); project != nil { return true } - return d.GroupConfig.ProjectExists(r) + return false } // EnsureCertificate parses the PEM-encoded certificate for the domain func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { + // TODO check len certificates instead of custom domain! if d.isUnconfigured() || !d.isCustomDomain() { return nil, errors.New("tls certificates can be loaded only for pages with configuration") } @@ -169,8 +183,8 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { d.certificateOnce.Do(func() { var cert tls.Certificate cert, d.certificateError = tls.X509KeyPair( - []byte(d.ProjectConfig.Certificate), - []byte(d.ProjectConfig.Key), + []byte(d.CertificateCert), + []byte(d.CertificateKey), ) if d.certificateError == nil { d.certificate = &cert @@ -182,26 +196,20 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { // ServeFileHTTP returns true if something was served, false if not. func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { - if d.isUnconfigured() { + if d.isUnconfigured() || !d.hasProject(r) { httperrors.Serve404(w) return true } - if !d.IsAccessControlEnabled(r) { - // Set caching headers - w.Header().Set("Cache-Control", "max-age=600") - w.Header().Set("Expires", time.Now().Add(10*time.Minute).Format(time.RFC1123)) - } - - return d.Serving().ServeFileHTTP(w, r) + return d.Serving().ServeFileHTTP(d.toHandler(w, r)) } // ServeNotFoundHTTP serves the not found pages from the projects. func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { - if d.isUnconfigured() { + if d.isUnconfigured() || !d.hasProject(r) { httperrors.Serve404(w) return } - d.Serving().ServeNotFoundHTTP(w, r) + d.Serving().ServeNotFoundHTTP(d.toHandler(w, r)) } diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 2be86e20e..8b3690754 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -11,10 +11,19 @@ import ( "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitlab-pages/internal/fixture" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) +type stubbedResolver struct { + project *Project + subpath string + err error +} + +func (resolver *stubbedResolver) Resolve(*http.Request) (*Project, string, error) { + return resolver.project, resolver.subpath, resolver.err +} + func serveFileOrNotFound(domain *Domain) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { if !domain.ServeFileHTTP(w, r) { @@ -23,27 +32,6 @@ func serveFileOrNotFound(domain *Domain) http.HandlerFunc { } } -func TestDomainServeHTTP(t *testing.T) { - cleanup := setUpTests(t) - defer cleanup() - - testDomain := &Domain{ - Project: "project2", - Group: "group", - ProjectConfig: &ProjectConfig{DomainName: "test.domain.com"}, - } - - require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/", nil, "project2-main") - require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/index.html", nil, "project2-main") - require.HTTPRedirect(t, serveFileOrNotFound(testDomain), "GET", "/subdir", nil) - require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/subdir", nil, - `Found`) - require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/subdir/", nil, "project2-subdir") - require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/subdir/index.html", nil, "project2-subdir") - require.HTTPError(t, serveFileOrNotFound(testDomain), "GET", "//about.gitlab.com/%2e%2e", nil) - require.HTTPError(t, serveFileOrNotFound(testDomain), "GET", "/not-existing-file", nil) -} - func TestIsHTTPSOnly(t *testing.T) { tests := []struct { name string @@ -54,9 +42,8 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only enabled", domain: &Domain{ - Project: "project", - Group: "group", - ProjectConfig: &ProjectConfig{HTTPSOnly: true}, + Location: "group/project", + Resolver: &stubbedResolver{project: &Project{IsHTTPSOnly: true}}, }, url: "http://custom-domain", expected: true, @@ -64,9 +51,8 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only disabled", domain: &Domain{ - Project: "project", - Group: "group", - ProjectConfig: &ProjectConfig{HTTPSOnly: false}, + Location: "group/project", + Resolver: &stubbedResolver{project: &Project{IsHTTPSOnly: false}}, }, url: "http://custom-domain", expected: false, @@ -74,8 +60,7 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Unknown project", domain: &Domain{ - Project: "project", - Group: "group", + Location: "group/project", }, url: "http://test-domain/project", expected: false, @@ -90,69 +75,6 @@ func TestIsHTTPSOnly(t *testing.T) { } } -func TestHasAcmeChallenge(t *testing.T) { - cleanup := setUpTests(t) - defer cleanup() - - tests := []struct { - name string - domain *Domain - token string - expected bool - }{ - { - name: "Project containing acme challenge", - domain: &Domain{ - Group: "group.acme", - Project: "with.acme.challenge", - ProjectConfig: &ProjectConfig{HTTPSOnly: true}, - }, - token: "existingtoken", - expected: true, - }, - { - name: "Project containing acme challenge", - domain: &Domain{ - Group: "group.acme", - Project: "with.acme.challenge", - ProjectConfig: &ProjectConfig{HTTPSOnly: true}, - }, - token: "foldertoken", - expected: true, - }, - { - name: "Project containing another token", - domain: &Domain{ - Group: "group.acme", - Project: "with.acme.challenge", - ProjectConfig: &ProjectConfig{HTTPSOnly: true}, - }, - token: "notexistingtoken", - expected: false, - }, - { - name: "nil domain", - domain: nil, - token: "existingtoken", - expected: false, - }, - { - name: "Domain without config", - domain: &Domain{ - Group: "group.acme", - Project: "with.acme.challenge", - }, - token: "existingtoken", - expected: false, - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - require.Equal(t, test.expected, test.domain.HasAcmeChallenge(test.token)) - }) - } -} - func testHTTPGzip(t *testing.T, handler http.HandlerFunc, mode, url string, values url.Values, acceptEncoding string, str interface{}, contentType string, ungzip bool) { w := httptest.NewRecorder() req, err := http.NewRequest(mode, url+"?"+values.Encode(), nil) @@ -180,26 +102,12 @@ func testHTTPGzip(t *testing.T, handler http.HandlerFunc, mode, url string, valu require.Equal(t, contentType, w.Header().Get("Content-Type")) } -func TestDomain404ServeHTTP(t *testing.T) { - cleanup := setUpTests(t) - defer cleanup() - - testDomain := &Domain{ - Group: "group.404", - Project: "domain.404", - ProjectConfig: &ProjectConfig{DomainName: "domain.404.com"}, - } - - testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") - testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") -} - func TestPredefined404ServeHTTP(t *testing.T) { cleanup := setUpTests(t) defer cleanup() testDomain := &Domain{ - Group: "group", + Location: "group", } testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found") @@ -207,8 +115,7 @@ func TestPredefined404ServeHTTP(t *testing.T) { func TestGroupCertificate(t *testing.T) { testGroup := &Domain{ - Project: "", - Group: "group", + Location: "group", } tls, err := testGroup.EnsureCertificate() @@ -218,9 +125,8 @@ func TestGroupCertificate(t *testing.T) { func TestDomainNoCertificate(t *testing.T) { testDomain := &Domain{ - Group: "group", - Project: "project2", - ProjectConfig: &ProjectConfig{DomainName: "test.domain.com"}, + Name: "test.domain.com", + Location: "group/project2", } tls, err := testDomain.EnsureCertificate() @@ -232,22 +138,6 @@ func TestDomainNoCertificate(t *testing.T) { require.Equal(t, err, err2) } -func TestDomainCertificate(t *testing.T) { - testDomain := &Domain{ - Group: "group", - Project: "project2", - ProjectConfig: &ProjectConfig{ - DomainName: "test.domain.com", - Certificate: fixture.Certificate, - Key: fixture.Key, - }, - } - - tls, err := testDomain.EnsureCertificate() - require.NotNil(t, tls) - require.NoError(t, err) -} - var chdirSet = false func setUpTests(t require.TestingT) func() { diff --git a/internal/domain/handler.go b/internal/domain/handler.go new file mode 100644 index 000000000..55697700d --- /dev/null +++ b/internal/domain/handler.go @@ -0,0 +1,42 @@ +package domain + +import "net/http" + +type handler struct { + writer http.ResponseWriter + request *http.Request + project *Project + subpath string +} + +func (h *handler) Writer() http.ResponseWriter { + return h.writer +} + +func (h *handler) Request() *http.Request { + return h.request +} + +func (h *handler) LookupPath() string { + return h.project.LookupPath +} + +func (h *handler) Subpath() string { + return h.subpath +} + +func (h *handler) IsNamespaceProject() bool { + return h.project.IsNamespaceProject +} + +func (h *handler) IsHTTPSOnly() bool { + return h.project.IsHTTPSOnly +} + +func (h *handler) HasAccessControl() bool { + return h.project.HasAccessControl +} + +func (h *handler) ProjectID() uint64 { + return h.project.ID +} diff --git a/internal/domain/project.go b/internal/domain/project.go new file mode 100644 index 000000000..9ea7306f8 --- /dev/null +++ b/internal/domain/project.go @@ -0,0 +1,10 @@ +package domain + +// Project holds a domain / project configuration +type Project struct { + LookupPath string + IsNamespaceProject bool + IsHTTPSOnly bool + HasAccessControl bool + ID uint64 +} diff --git a/internal/domain/resolver.go b/internal/domain/resolver.go new file mode 100644 index 000000000..5bde31ec2 --- /dev/null +++ b/internal/domain/resolver.go @@ -0,0 +1,10 @@ +package domain + +import "net/http" + +// Resolver represents an interface responsible for resolving a project +// per-request +type Resolver interface { + // Resolve returns a project with a file path and an error if it occured + Resolve(*http.Request) (*Project, string, error) +} diff --git a/internal/serving/disk.go b/internal/serving/disk.go new file mode 100644 index 000000000..b569986fe --- /dev/null +++ b/internal/serving/disk.go @@ -0,0 +1,19 @@ +package serving + +import "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" + +type diskServing struct { + disk *disk.Serving +} + +func (d *diskServing) ServeFileHTTP(h Handler) bool { + return d.disk.ServeFileHTTP(h) +} + +func (d *diskServing) ServeNotFoundHTTP(h Handler) { + d.disk.ServeNotFoundHTTP(h) +} + +func (d *diskServing) HasAcmeChallenge(h Handler, token string) bool { + return d.disk.HasAcmeChallenge(h, token) +} diff --git a/internal/serving/disk/group.go b/internal/serving/disk/group.go deleted file mode 100644 index 78a4ff1f8..000000000 --- a/internal/serving/disk/group.go +++ /dev/null @@ -1,64 +0,0 @@ -package disk - -import ( - "net/http" - - "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" -) - -// Group serving represents a resource that can be served from a directory -// representing GitLab group -type Group struct { - Resolver - *Reader -} - -type Resolver interface { - ProjectWithSubpath(*http.Request) (string, string, error) -} - -// ServeFileHTTP returns true if something was served, false if not. -func (g *Group) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { - return g.serveFileFromGroup(w, r) -} - -// ServeNotFoundHTTP serves the not found pages from the projects. -func (g *Group) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { - g.serveNotFoundFromGroup(w, r) -} - -func (g *Group) HasAcmeChallenge(token string) bool { - return false -} - -func (g *Group) serveFileFromGroup(w http.ResponseWriter, r *http.Request) bool { - projectName, subPath, err := g.Resolver.ProjectWithSubpath(r) - - if err != nil { - httperrors.Serve404(w) - return true - } - - if g.tryFile(w, r, projectName, subPath) == nil { - return true - } - - return false -} - -func (g *Group) serveNotFoundFromGroup(w http.ResponseWriter, r *http.Request) { - projectName, _, err := g.Resolver.ProjectWithSubpath(r) - - if err != nil { - httperrors.Serve404(w) - return - } - - // Try serving custom not-found page - if g.tryNotFound(w, r, projectName) == nil { - return - } - - // Generic 404 - httperrors.Serve404(w) -} diff --git a/internal/serving/disk/handler.go b/internal/serving/disk/handler.go new file mode 100644 index 000000000..fbbf9ce3b --- /dev/null +++ b/internal/serving/disk/handler.go @@ -0,0 +1,11 @@ +package disk + +import "net/http" + +type handler interface { + Writer() http.ResponseWriter + Request() *http.Request + LookupPath() string + Subpath() string + HasAccessControl() bool +} diff --git a/internal/serving/disk/project.go b/internal/serving/disk/project.go deleted file mode 100644 index b2940f3d3..000000000 --- a/internal/serving/disk/project.go +++ /dev/null @@ -1,58 +0,0 @@ -package disk - -import ( - "net/http" - - "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" -) - -// Custom serving represent a resource that can be served from a directory -// representing GitLab project -type Project struct { - Location string - *Reader -} - -// ServeFileHTTP returns true if something was served, false if not. -func (p *Project) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { - return p.serveFileFromConfig(w, r) -} - -// ServeNotFoundHTTP serves the not found pages from the projects. -func (p *Project) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { - p.serveNotFoundFromConfig(w, r) -} - -func (p *Project) HasAcmeChallenge(token string) bool { - _, err := p.resolvePath(p.Location, ".well-known/acme-challenge", token) - // there is an acme challenge on disk - if err == nil { - return true - } - - _, err = p.resolvePath(p.Location, ".well-known/acme-challenge", token, "index.html") - if err == nil { - return true - } - - return false -} - -func (p *Project) serveFileFromConfig(w http.ResponseWriter, r *http.Request) bool { - // Try to serve file for http://host/... => /group/project/... - if p.tryFile(w, r, p.Location, r.URL.Path) == nil { - return true - } - - return false -} - -func (p *Project) serveNotFoundFromConfig(w http.ResponseWriter, r *http.Request) { - // Try serving not found page for http://host/ => /group/project/404.html - if p.tryNotFound(w, r, p.Location) == nil { - return - } - - // Serve generic not found - httperrors.Serve404(w) -} diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index 068b4b47f..1fc30da7d 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -8,48 +8,54 @@ import ( "path/filepath" "strconv" "strings" + "time" ) +// Reader is a disk access driver type Reader struct { - Group string + Location string } -func (reader *Reader) tryFile(w http.ResponseWriter, r *http.Request, projectName string, subPath ...string) error { - fullPath, err := reader.resolvePath(projectName, subPath...) +func (reader *Reader) tryFile(h handler) error { + fullPath, err := reader.resolvePath(h.LookupPath(), h.Subpath()) + + request := h.Request() + host := request.Host + urlPath := request.URL.Path if locationError, _ := err.(*locationDirectoryError); locationError != nil { - if endsWithSlash(r.URL.Path) { - fullPath, err = reader.resolvePath(projectName, filepath.Join(subPath...), "index.html") + if endsWithSlash(urlPath) { + fullPath, err = reader.resolvePath(h.LookupPath(), h.Subpath(), "index.html") } else { // Concat Host with URL.Path - redirectPath := "//" + r.Host + "/" - redirectPath += strings.TrimPrefix(r.URL.Path, "/") + redirectPath := "//" + host + "/" + redirectPath += strings.TrimPrefix(urlPath, "/") // Ensure that there's always "/" at end redirectPath = strings.TrimSuffix(redirectPath, "/") + "/" - http.Redirect(w, r, redirectPath, 302) + http.Redirect(h.Writer(), h.Request(), redirectPath, 302) return nil } } if locationError, _ := err.(*locationFileNoExtensionError); locationError != nil { - fullPath, err = reader.resolvePath(projectName, strings.TrimSuffix(filepath.Join(subPath...), "/")+".html") + fullPath, err = reader.resolvePath(h.LookupPath(), strings.TrimSuffix(h.Subpath(), "/")+".html") } if err != nil { return err } - return reader.serveFile(w, r, fullPath) + return reader.serveFile(h.Writer(), h.Request(), fullPath, h.HasAccessControl()) } -func (reader *Reader) tryNotFound(w http.ResponseWriter, r *http.Request, projectName string) error { - page404, err := reader.resolvePath(projectName, "404.html") +func (reader *Reader) tryNotFound(h handler) error { + page404, err := reader.resolvePath(h.LookupPath(), "404.html") if err != nil { return err } - err = reader.serveCustomFile(w, r, http.StatusNotFound, page404) + err = reader.serveCustomFile(h.Writer(), h.Request(), http.StatusNotFound, page404) if err != nil { return err } @@ -58,8 +64,8 @@ func (reader *Reader) tryNotFound(w http.ResponseWriter, r *http.Request, projec // Resolve the HTTP request to a path on disk, converting requests for // directories to requests for index.html inside the directory if appropriate. -func (reader *Reader) resolvePath(projectName string, subPath ...string) (string, error) { - publicPath := filepath.Join(reader.Group, projectName, "public") +func (reader *Reader) resolvePath(lookupPath string, subPath ...string) (string, error) { + publicPath := filepath.Join(reader.Location, lookupPath, "public") // Don't use filepath.Join as cleans the path, // where we want to traverse full path as supplied by user @@ -103,7 +109,7 @@ func (reader *Reader) resolvePath(projectName string, subPath ...string) (string return fullPath, nil } -func (reader *Reader) serveFile(w http.ResponseWriter, r *http.Request, origPath string) error { +func (reader *Reader) serveFile(w http.ResponseWriter, r *http.Request, origPath string, accessControl bool) error { fullPath := handleGZip(w, r, origPath) file, err := openNoFollow(fullPath) @@ -118,6 +124,12 @@ func (reader *Reader) serveFile(w http.ResponseWriter, r *http.Request, origPath return err } + if !accessControl { + // Set caching headers + w.Header().Set("Cache-Control", "max-age=600") + w.Header().Set("Expires", time.Now().Add(10*time.Minute).Format(time.RFC1123)) + } + contentType, err := detectContentType(origPath) if err != nil { return err diff --git a/internal/serving/disk/serving.go b/internal/serving/disk/serving.go new file mode 100644 index 000000000..78b1b5721 --- /dev/null +++ b/internal/serving/disk/serving.go @@ -0,0 +1,47 @@ +package disk + +import ( + "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" +) + +// Serving describes a disk access serving +type Serving struct { + Domain string // TODO it is not used but might be handy + *Reader +} + +// ServeFileHTTP serves a file from disk and returns true. It returns false +// when a file could not been found. +func (s *Serving) ServeFileHTTP(h handler) bool { + if s.tryFile(h) == nil { + return true + } + + return false +} + +// ServeNotFoundHTTP tries to read a custom 404 page +func (s *Serving) ServeNotFoundHTTP(h handler) { + if s.tryNotFound(h) == nil { + return + } + + // Generic 404 + httperrors.Serve404(h.Writer()) +} + +// HasAcmeChallenge checks if the ACME challenge is present on the disk +func (s *Serving) HasAcmeChallenge(h handler, token string) bool { + _, err := s.resolvePath(h.LookupPath(), ".well-known/acme-challenge", token) + // there is an acme challenge on disk + if err == nil { + return true + } + + _, err = s.resolvePath(h.LookupPath(), ".well-known/acme-challenge", token, "index.html") + if err == nil { + return true + } + + return false +} diff --git a/internal/serving/handler.go b/internal/serving/handler.go new file mode 100644 index 000000000..2a9969b7b --- /dev/null +++ b/internal/serving/handler.go @@ -0,0 +1,16 @@ +package serving + +import "net/http" + +// Handler interface represent an interface that is needed to fullfil the +// serving request +type Handler interface { + Writer() http.ResponseWriter + Request() *http.Request + LookupPath() string + Subpath() string + IsNamespaceProject() bool + IsHTTPSOnly() bool + HasAccessControl() bool + ProjectID() uint64 +} diff --git a/internal/serving/serving.go b/internal/serving/serving.go index b28d09001..c6a392411 100644 --- a/internal/serving/serving.go +++ b/internal/serving/serving.go @@ -1,32 +1,23 @@ package serving import ( - "net/http" - "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" ) // Serving is an interface used to define a serving driver type Serving interface { - ServeFileHTTP(http.ResponseWriter, *http.Request) bool - ServeNotFoundHTTP(http.ResponseWriter, *http.Request) - HasAcmeChallenge(token string) bool -} - -func NewProjectDiskServing(project, group string) Serving { - return &disk.Project{ - Location: project, - Reader: &disk.Reader{ - Group: group, - }, - } + ServeFileHTTP(Handler) bool + ServeNotFoundHTTP(Handler) + HasAcmeChallenge(handler Handler, token string) bool } -func NewGroupDiskServing(group string, resolver disk.Resolver) Serving { - return &disk.Group{ - Resolver: resolver, - Reader: &disk.Reader{ - Group: group, +// NewDiskServing returns a serving instance that is capable of reading files +// from the disk +func NewDiskServing(domain, location string) Serving { + return &diskServing{ + disk: &disk.Serving{ + Domain: domain, + Reader: &disk.Reader{Location: location}, }, } } diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go new file mode 100644 index 000000000..0cf443b0c --- /dev/null +++ b/internal/source/disk/custom.go @@ -0,0 +1,24 @@ +package disk + +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" +) + +type customProjectResolver struct { + config *domainConfig +} + +// TODO tests +func (p *customProjectResolver) Resolve(r *http.Request) (*domain.Project, string, error) { + project := &domain.Project{ + LookupPath: "/", + IsNamespaceProject: false, + IsHTTPSOnly: p.config.HTTPSOnly, + HasAccessControl: p.config.AccessControl, + ID: p.config.ID, + } + + return project, r.URL.Path, nil +} diff --git a/internal/source/disk/group_domain_test.go b/internal/source/disk/domain_test.go similarity index 92% rename from internal/source/disk/group_domain_test.go rename to internal/source/disk/domain_test.go index 3b4471f4f..80772d374 100644 --- a/internal/source/disk/group_domain_test.go +++ b/internal/source/disk/domain_test.go @@ -27,9 +27,8 @@ func serveFileOrNotFound(domain *domain.Domain) http.HandlerFunc { func testGroupServeHTTPHost(t *testing.T, host string) { testGroup := &domain.Domain{ - Project: "", - Group: "group", - GroupConfig: &Group{ + Location: "group", + Resolver: &Group{ name: "group", projects: map[string]*projectConfig{ "group.test.io": &projectConfig{}, @@ -82,9 +81,11 @@ func TestDomainServeHTTP(t *testing.T) { defer cleanup() testDomain := &domain.Domain{ - Group: "group", - Project: "project2", - ProjectConfig: &domain.ProjectConfig{DomainName: "test.domain.com"}, + Name: "test.domain.com", + Location: "group/project2", + Resolver: &customProjectResolver{ + config: &domainConfig{}, + }, } require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/", nil, "project2-main") @@ -108,9 +109,8 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Default group domain with HTTPS-only enabled", domain: &domain.Domain{ - Group: "group", - Project: "project", - GroupConfig: &Group{ + Location: "group/project", + Resolver: &Group{ name: "group", projects: projects{"test-domain": &projectConfig{HTTPSOnly: true}}, }, @@ -121,9 +121,8 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Default group domain with HTTPS-only disabled", domain: &domain.Domain{ - Group: "group", - Project: "project", - GroupConfig: &Group{ + Location: "group/project", + Resolver: &Group{ name: "group", projects: projects{"test-domain": &projectConfig{HTTPSOnly: false}}, }, @@ -134,9 +133,8 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Case-insensitive default group domain with HTTPS-only enabled", domain: &domain.Domain{ - Project: "project", - Group: "group", - GroupConfig: &Group{ + Location: "group/project", + Resolver: &Group{ name: "group", projects: projects{"test-domain": &projectConfig{HTTPSOnly: true}}, }, @@ -147,9 +145,8 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Other group domain with HTTPS-only enabled", domain: &domain.Domain{ - Project: "project", - Group: "group", - GroupConfig: &Group{ + Location: "group/project", + Resolver: &Group{ name: "group", projects: projects{"project": &projectConfig{HTTPSOnly: true}}, }, @@ -160,9 +157,8 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Other group domain with HTTPS-only disabled", domain: &domain.Domain{ - Project: "project", - Group: "group", - GroupConfig: &Group{ + Location: "group/project", + Resolver: &Group{ name: "group", projects: projects{"project": &projectConfig{HTTPSOnly: false}}, }, @@ -173,8 +169,7 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Unknown project", domain: &domain.Domain{ - Group: "group", - Project: "project", + Location: "group/project", }, url: "http://test-domain/project", expected: false, @@ -221,9 +216,8 @@ func TestGroupServeHTTPGzip(t *testing.T) { defer cleanup() testGroup := &domain.Domain{ - Project: "", - Group: "group", - GroupConfig: &Group{ + Location: "group", + Resolver: &Group{ name: "group", projects: map[string]*projectConfig{ "group.test.io": &projectConfig{}, @@ -289,9 +283,8 @@ func TestGroup404ServeHTTP(t *testing.T) { defer cleanup() testGroup := &domain.Domain{ - Project: "", - Group: "group.404", - GroupConfig: &Group{ + Location: "group.404", + Resolver: &Group{ name: "group.404", projects: map[string]*projectConfig{ "domain.404": &projectConfig{}, @@ -319,9 +312,10 @@ func TestDomain404ServeHTTP(t *testing.T) { defer cleanup() testDomain := &domain.Domain{ - Project: "domain.404", - Group: "group.404", - ProjectConfig: &domain.ProjectConfig{DomainName: "domain.404.com"}, + Location: "group.404/domain.404", + Resolver: &customProjectResolver{ + config: &domainConfig{Domain: "domain.404.com"}, + }, } testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") @@ -333,7 +327,7 @@ func TestPredefined404ServeHTTP(t *testing.T) { defer cleanup() testDomain := &domain.Domain{ - Group: "group", + Location: "group", } testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found") @@ -341,8 +335,7 @@ func TestPredefined404ServeHTTP(t *testing.T) { func TestGroupCertificate(t *testing.T) { testGroup := &domain.Domain{ - Group: "group", - Project: "", + Location: "group", } tls, err := testGroup.EnsureCertificate() @@ -352,9 +345,10 @@ func TestGroupCertificate(t *testing.T) { func TestDomainNoCertificate(t *testing.T) { testDomain := &domain.Domain{ - Group: "group", - Project: "project2", - ProjectConfig: &domain.ProjectConfig{DomainName: "test.domain.com"}, + Location: "group/project2", + Resolver: &customProjectResolver{ + config: &domainConfig{Domain: "test.domain.com"}, + }, } tls, err := testDomain.EnsureCertificate() @@ -368,12 +362,12 @@ func TestDomainNoCertificate(t *testing.T) { func TestDomainCertificate(t *testing.T) { testDomain := &domain.Domain{ - Project: "project2", - Group: "group", - ProjectConfig: &domain.ProjectConfig{DomainName: "test.domain.com", - Certificate: fixture.Certificate, - Key: fixture.Key, - }, + Customized: true, // TODO + Location: "group/project2", + Name: "test.domain.com", + CertificateCert: fixture.Certificate, + CertificateKey: fixture.Key, + Resolver: &customProjectResolver{}, } tls, err := testDomain.EnsureCertificate() @@ -386,8 +380,8 @@ func TestCacheControlHeaders(t *testing.T) { defer cleanup() testGroup := &domain.Domain{ - Group: "group", - GroupConfig: &Group{ + Location: "group", + Resolver: &Group{ name: "group", projects: map[string]*projectConfig{ "group.test.io": &projectConfig{}, diff --git a/internal/source/disk/group.go b/internal/source/disk/group.go index 0c8d08104..efa3fce5b 100644 --- a/internal/source/disk/group.go +++ b/internal/source/disk/group.go @@ -1,11 +1,11 @@ package disk import ( - "errors" "net/http" "path" "strings" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/host" ) @@ -74,72 +74,22 @@ func (g *Group) getProjectConfigWithSubpath(r *http.Request) (*projectConfig, st return nil, "", "" } -// IsHTTPSOnly return true if project exists and has https-only setting -// configured -func (g *Group) IsHTTPSOnly(r *http.Request) bool { - project, _, _ := g.getProjectConfigWithSubpath(r) +// Resolve tries to find project and its config recursively for a given request +// to a group domain +func (g *Group) Resolve(r *http.Request) (*domain.Project, string, error) { + projectConfig, projectPath, subPath := g.getProjectConfigWithSubpath(r) - if project != nil { - return project.HTTPSOnly + if projectConfig == nil { + return nil, "", nil // it is not an error when project does not exist } - return false -} - -// HasAccessControl returns true if a group project has access control setting -// enabled -func (g *Group) HasAccessControl(r *http.Request) bool { - project, _, _ := g.getProjectConfigWithSubpath(r) - - if project != nil { - return project.AccessControl - } - - return false -} - -// IsNamespaceProject return true if per-request config belongs to a namespace -// project -func (g *Group) IsNamespaceProject(r *http.Request) bool { - project, _, _ := g.getProjectConfigWithSubpath(r) - - if project != nil { - return project.NamespaceProject - } - - return false -} - -// ProjectID return a per-request group project ID -func (g *Group) ProjectID(r *http.Request) uint64 { - project, _, _ := g.getProjectConfigWithSubpath(r) - - if project != nil { - return project.ID - } - - return 0 -} - -// ProjectExists return true if project config has been found -func (g *Group) ProjectExists(r *http.Request) bool { - project, _, _ := g.getProjectConfigWithSubpath(r) - - if project != nil { - return true - } - - return false -} - -// ProjectWithSubpath tries to find project and its config recursively for a -// given request to a group domain -func (g *Group) ProjectWithSubpath(r *http.Request) (string, string, error) { - project, projectName, subPath := g.getProjectConfigWithSubpath(r) - - if project != nil { - return projectName, subPath, nil + project := &domain.Project{ + LookupPath: projectPath, + IsNamespaceProject: projectConfig.NamespaceProject, + IsHTTPSOnly: projectConfig.HTTPSOnly, + HasAccessControl: projectConfig.AccessControl, + ID: projectConfig.ID, } - return "", "", errors.New("project not found") + return project, subPath, nil } diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index dd368501c..c330a2ea9 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -24,11 +24,9 @@ type domainsUpdater func(Map) func (dm Map) updateDomainMap(domainName string, domain *domain.Domain) { if old, ok := dm[domainName]; ok { log.WithFields(log.Fields{ - "domain_name": domainName, - "new_group": domain.Group, - "new_project_name": domain.Project, - "old_group": old.Group, - "old_project_name": old.Project, + "domain_name": domainName, + "new_location": domain.Location, + "old_location": old.Location, }).Error("Duplicate domain") } @@ -37,21 +35,15 @@ func (dm Map) updateDomainMap(domainName string, domain *domain.Domain) { func (dm Map) addDomain(rootDomain, groupName, projectName string, config *domainConfig) { newDomain := &domain.Domain{ - Group: groupName, - Project: projectName, - ProjectConfig: &domain.ProjectConfig{ - DomainName: config.Domain, - Certificate: config.Certificate, - Key: config.Key, - HTTPSOnly: config.HTTPSOnly, - ProjectID: config.ID, - AccessControl: config.AccessControl, - }, + Name: strings.ToLower(config.Domain), + Customized: true, // TODO remove + CertificateCert: config.Certificate, + CertificateKey: config.Key, + Location: filepath.Join(groupName, projectName), + Resolver: &customProjectResolver{config: config}, } - var domainName string - domainName = strings.ToLower(config.Domain) - dm.updateDomainMap(domainName, newDomain) + dm.updateDomainMap(newDomain.Name, newDomain) } func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, httpsOnly bool, accessControl bool, id uint64) { @@ -59,21 +51,23 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https groupDomain := dm[domainName] if groupDomain == nil { - group := &Group{ + groupResolver := &Group{ name: groupName, projects: make(projects), subgroups: make(subgroups), } groupDomain = &domain.Domain{ - Group: groupName, - GroupConfig: group, + Name: domainName, + Customized: false, // TODO remove + Location: groupName, + Resolver: groupResolver, } } split := strings.SplitN(strings.ToLower(projectPath), "/", maxProjectDepth) projectName := split[len(split)-1] - g := groupDomain.GroupConfig.(*Group) + g := groupDomain.Resolver.(*Group) for i := 0; i < len(split)-1; i++ { subgroupName := split[i] diff --git a/internal/source/disk/map_test.go b/internal/source/disk/map_test.go index 9aa960722..c15f29c61 100644 --- a/internal/source/disk/map_test.go +++ b/internal/source/disk/map_test.go @@ -68,15 +68,15 @@ func TestReadProjects(t *testing.T) { } // Check that multiple domains in the same project are recorded faithfully - require.Equal(t, "test.domain.com", dm["test.domain.com"].ProjectConfig.DomainName) - require.Equal(t, "other.domain.com", dm["other.domain.com"].ProjectConfig.DomainName) - require.Equal(t, "test", dm["other.domain.com"].ProjectConfig.Certificate) - require.Equal(t, "key", dm["other.domain.com"].ProjectConfig.Key) + require.Equal(t, "test.domain.com", dm["test.domain.com"].Name) + require.Equal(t, "other.domain.com", dm["other.domain.com"].Name) + require.Equal(t, "test", dm["other.domain.com"].CertificateCert) + require.Equal(t, "key", dm["other.domain.com"].CertificateKey) // check subgroups domain, ok := dm["group.test.io"] require.True(t, ok, "missing group.test.io domain") - subgroup, ok := domain.GroupConfig.(*Group).subgroups["subgroup"] + subgroup, ok := domain.Resolver.(*Group).subgroups["subgroup"] require.True(t, ok, "missing group.test.io subgroup") _, ok = subgroup.projects["project"] require.True(t, ok, "missing project for subgroup in group.test.io domain") @@ -117,7 +117,7 @@ func TestReadProjectsMaxDepth(t *testing.T) { // check subgroups domain, ok := dm["group-0.test.io"] require.True(t, ok, "missing group-0.test.io domain") - subgroup := domain.GroupConfig.(*Group) + subgroup := domain.Resolver.(*Group) for i := 0; i < levels; i++ { subgroup, ok = subgroup.subgroups["sub"] if i <= subgroupScanLimit { -- GitLab From f025a4ffe992051054a64d91c28408ff6916f4f4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 27 Sep 2019 11:58:18 +0200 Subject: [PATCH 08/17] Reduce duplication the domain type --- internal/domain/domain.go | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 1dd5be51e..321c0cd91 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -62,7 +62,7 @@ func (d *Domain) resolve(r *http.Request) (*Project, string) { return project, subpath } -func (d *Domain) getProject(r *http.Request) *Project { +func (d *Domain) GetProject(r *http.Request) *Project { project, _ := d.resolve(r) return project @@ -88,10 +88,6 @@ func (d *Domain) toHandler(w http.ResponseWriter, r *http.Request) *handler { } } -func (d *Domain) hasProject(r *http.Request) bool { - return d.getProject(r) != nil -} - // IsHTTPSOnly figures out if the request should be handled with HTTPS // only by looking at group and project level config. func (d *Domain) IsHTTPSOnly(r *http.Request) bool { @@ -99,7 +95,7 @@ func (d *Domain) IsHTTPSOnly(r *http.Request) bool { return false } - if project := d.getProject(r); project != nil { + if project := d.GetProject(r); project != nil { return project.IsHTTPSOnly } @@ -112,7 +108,7 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { return false } - if project := d.getProject(r); project != nil { + if project := d.GetProject(r); project != nil { return project.HasAccessControl } @@ -121,7 +117,7 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { // HasAcmeChallenge checks domain directory contains particular acme challenge func (d *Domain) HasAcmeChallenge(r *http.Request, token string) bool { - if d.isUnconfigured() || !d.isCustomDomain() || !d.hasProject(r) { + if d.isUnconfigured() || !d.isCustomDomain() || !d.HasProject(r) { return false } @@ -140,7 +136,7 @@ func (d *Domain) IsNamespaceProject(r *http.Request) bool { return false } - if project := d.getProject(r); project != nil { + if project := d.GetProject(r); project != nil { return project.IsNamespaceProject } @@ -153,7 +149,7 @@ func (d *Domain) GetID(r *http.Request) uint64 { return 0 } - if project := d.getProject(r); project != nil { + if project := d.GetProject(r); project != nil { return project.ID } @@ -166,11 +162,7 @@ func (d *Domain) HasProject(r *http.Request) bool { return false } - if project := d.getProject(r); project != nil { - return true - } - - return false + return d.GetProject(r) != nil } // EnsureCertificate parses the PEM-encoded certificate for the domain @@ -196,7 +188,7 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { // ServeFileHTTP returns true if something was served, false if not. func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { - if d.isUnconfigured() || !d.hasProject(r) { + if d.isUnconfigured() || !d.HasProject(r) { httperrors.Serve404(w) return true } @@ -206,7 +198,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { // ServeNotFoundHTTP serves the not found pages from the projects. func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { - if d.isUnconfigured() || !d.hasProject(r) { + if d.isUnconfigured() || !d.HasProject(r) { httperrors.Serve404(w) return } -- GitLab From 06aeccc03e3626c10c6a0c87e917d21125d6e8a8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 27 Sep 2019 12:03:29 +0200 Subject: [PATCH 09/17] Remove the concept of a custom domain from domain package --- internal/domain/domain.go | 22 ++++------------------ internal/source/disk/domain_test.go | 1 - internal/source/disk/map.go | 8 +++----- 3 files changed, 7 insertions(+), 24 deletions(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 321c0cd91..d57687a9c 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -16,7 +16,6 @@ type Domain struct { Location string CertificateCert string CertificateKey string - Customized bool // TODO we should get rid of this Resolver Resolver @@ -33,14 +32,6 @@ func (d *Domain) String() string { return d.Name } -func (d *Domain) isCustomDomain() bool { - if d.isUnconfigured() { - panic("project config and group config should not be nil at the same time") - } - - return d.Customized -} - func (d *Domain) isUnconfigured() bool { if d == nil { return true @@ -62,6 +53,7 @@ func (d *Domain) resolve(r *http.Request) (*Project, string) { return project, subpath } +// GetProject returns a project details based on the request func (d *Domain) GetProject(r *http.Request) *Project { project, _ := d.resolve(r) @@ -117,7 +109,8 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { // HasAcmeChallenge checks domain directory contains particular acme challenge func (d *Domain) HasAcmeChallenge(r *http.Request, token string) bool { - if d.isUnconfigured() || !d.isCustomDomain() || !d.HasProject(r) { + // TODO is that safe to redirect to acme challenge in GitLab if it is a grup domain/ + if d.isUnconfigured() || !d.HasProject(r) { return false } @@ -130,12 +123,6 @@ func (d *Domain) IsNamespaceProject(r *http.Request) bool { return false } - // If request is to a custom domain, we do not handle it as a namespace project - // as there can't be multiple projects under the same custom domain - if d.isCustomDomain() { // TODO do we need a separate path for this - return false - } - if project := d.GetProject(r); project != nil { return project.IsNamespaceProject } @@ -167,8 +154,7 @@ func (d *Domain) HasProject(r *http.Request) bool { // EnsureCertificate parses the PEM-encoded certificate for the domain func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { - // TODO check len certificates instead of custom domain! - if d.isUnconfigured() || !d.isCustomDomain() { + if d.isUnconfigured() || len(d.CertificateKey) == 0 || len(d.CertificateCert) == 0 { return nil, errors.New("tls certificates can be loaded only for pages with configuration") } diff --git a/internal/source/disk/domain_test.go b/internal/source/disk/domain_test.go index 80772d374..4140241e6 100644 --- a/internal/source/disk/domain_test.go +++ b/internal/source/disk/domain_test.go @@ -362,7 +362,6 @@ func TestDomainNoCertificate(t *testing.T) { func TestDomainCertificate(t *testing.T) { testDomain := &domain.Domain{ - Customized: true, // TODO Location: "group/project2", Name: "test.domain.com", CertificateCert: fixture.Certificate, diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index c330a2ea9..09902241f 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -36,7 +36,6 @@ func (dm Map) updateDomainMap(domainName string, domain *domain.Domain) { func (dm Map) addDomain(rootDomain, groupName, projectName string, config *domainConfig) { newDomain := &domain.Domain{ Name: strings.ToLower(config.Domain), - Customized: true, // TODO remove CertificateCert: config.Certificate, CertificateKey: config.Key, Location: filepath.Join(groupName, projectName), @@ -58,10 +57,9 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https } groupDomain = &domain.Domain{ - Name: domainName, - Customized: false, // TODO remove - Location: groupName, - Resolver: groupResolver, + Name: domainName, + Location: groupName, + Resolver: groupResolver, } } -- GitLab From 8d750c396826c66b472a9bc4d7aee4fe097833c7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 27 Sep 2019 12:15:14 +0200 Subject: [PATCH 10/17] Improve comments and TODOs left for future iterations --- go.mod | 2 -- internal/domain/domain.go | 13 +++++++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 046398e28..2c4252a45 100644 --- a/go.mod +++ b/go.mod @@ -13,8 +13,6 @@ require ( github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 github.com/karrick/godirwalk v1.10.12 - github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect - github.com/modern-go/reflect2 v1.0.1 // indirect github.com/namsral/flag v1.7.4-pre github.com/prometheus/client_golang v1.1.0 github.com/rs/cors v1.7.0 diff --git a/internal/domain/domain.go b/internal/domain/domain.go index d57687a9c..df484dd2c 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -41,11 +41,12 @@ func (d *Domain) isUnconfigured() bool { } func (d *Domain) resolve(r *http.Request) (*Project, string) { - // TODO use lookupPaths first + // TODO use lookupPaths to cache information about projects better, to + // improve performance and resilience project, subpath, _ := d.Resolver.Resolve(r) - // current implementation does not return errors in any case + // Current implementation does not return errors in any case if project == nil { return nil, "" } @@ -109,12 +110,16 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { // HasAcmeChallenge checks domain directory contains particular acme challenge func (d *Domain) HasAcmeChallenge(r *http.Request, token string) bool { - // TODO is that safe to redirect to acme challenge in GitLab if it is a grup domain/ + // TODO is that safe to redirect to acme challenge in GitLab if it is a grup domain? if d.isUnconfigured() || !d.HasProject(r) { return false } - return d.Serving().HasAcmeChallenge(d.toHandler(nil, r), token) // TODO + // TODO we should improve that, we need different type of information to + // check if the ACME challenge is present in the serving. We should devise a + // better interface here or we should to extract this responsibility + // somewhere else. + return d.Serving().HasAcmeChallenge(d.toHandler(nil, r), token) } // IsNamespaceProject figures out if the request is to a namespace project -- GitLab From 0a89a48ec93a6f9d52df719c82af5d765bb45d10 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 27 Sep 2019 11:07:16 +0200 Subject: [PATCH 11/17] Create a factory method for Domains to handle mutex better --- app.go | 2 +- internal/auth/auth_test.go | 8 ++++---- internal/source/domains.go | 11 ++++++++++- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/app.go b/app.go index 0d69ac797..67b94f2ad 100644 --- a/app.go +++ b/app.go @@ -461,7 +461,7 @@ func (a *theApp) listenAdminHTTPS(wg *sync.WaitGroup) { } func runApp(config appConfig) { - a := theApp{appConfig: config, domains: new(source.Domains)} + a := theApp{appConfig: config, domains: source.NewDomains()} err := logging.ConfigureLogging(a.LogFormat, a.LogVerbose) if err != nil { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index e8ff5e94f..ad6550ac7 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -54,7 +54,7 @@ func TestTryAuthenticate(t *testing.T) { require.NoError(t, err) r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true) - require.Equal(t, false, auth.TryAuthenticate(result, r, new(source.Domains))) + require.Equal(t, false, auth.TryAuthenticate(result, r, source.NewDomains())) } func TestTryAuthenticateWithError(t *testing.T) { @@ -65,7 +65,7 @@ func TestTryAuthenticateWithError(t *testing.T) { require.NoError(t, err) r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true) - require.Equal(t, true, auth.TryAuthenticate(result, r, new(source.Domains))) + require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewDomains())) require.Equal(t, 401, result.Code) } @@ -82,7 +82,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) { session.Values["state"] = "state" session.Save(r, result) - require.Equal(t, true, auth.TryAuthenticate(result, r, new(source.Domains))) + require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewDomains())) require.Equal(t, 401, result.Code) } @@ -122,7 +122,7 @@ func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) { }) result := httptest.NewRecorder() - require.Equal(t, true, auth.TryAuthenticate(result, r, new(source.Domains))) + require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewDomains())) require.Equal(t, 302, result.Code) require.Equal(t, "https://pages.gitlab-example.com/project/", result.Header().Get("Location")) require.Equal(t, 600, result.Result().Cookies()[0].MaxAge) diff --git a/internal/source/domains.go b/internal/source/domains.go index 54a269d86..85646b8a0 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -13,7 +13,16 @@ import ( // currently reading them from disk. type Domains struct { dm disk.Map - lock sync.RWMutex + lock *sync.RWMutex +} + +// NewDomains is a factory method for domains initializing a mutex. It should +// not initialize `dm` as we later check the readiness by comparing it with a +// nil value. +func NewDomains() *Domains { + return &Domains{ + lock: new(sync.RWMutex), + } } // GetDomain returns a domain from the domains map -- GitLab From 4b49d1504cf0be626c3b09f18f8f70a6c5dfbc84 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 27 Sep 2019 12:38:34 +0200 Subject: [PATCH 12/17] Fix test case names in config_test.go --- internal/source/disk/config_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/source/disk/config_test.go b/internal/source/disk/config_test.go index ec2bb8548..1bb2364ab 100644 --- a/internal/source/disk/config_test.go +++ b/internal/source/disk/config_test.go @@ -13,7 +13,7 @@ const configFile = "test-group/test-project/config.json" const invalidConfig = `{"Domains":{}}` const validConfig = `{"Domains":[{"Domain":"test"}]}` -func TestdomainConfigValidness(t *testing.T) { +func TestDomainConfigValidness(t *testing.T) { d := domainConfig{} require.False(t, d.Valid("gitlab.io")) @@ -36,7 +36,7 @@ func TestdomainConfigValidness(t *testing.T) { require.False(t, d.Valid("gitlab.io")) } -func TestdomainConfigRead(t *testing.T) { +func TestDomainConfigRead(t *testing.T) { cleanup := setUpTests(t) defer cleanup() -- GitLab From 9660a822ed42dd28fa1d7e878a1a7f05c2b351b5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 27 Sep 2019 12:39:07 +0200 Subject: [PATCH 13/17] Fix imports orderding in disk helpers.go file --- internal/serving/disk/helpers.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/serving/disk/helpers.go b/internal/serving/disk/helpers.go index 7ace39be2..01f597673 100644 --- a/internal/serving/disk/helpers.go +++ b/internal/serving/disk/helpers.go @@ -8,8 +8,9 @@ import ( "path/filepath" "strings" - "gitlab.com/gitlab-org/gitlab-pages/internal/httputil" "golang.org/x/sys/unix" + + "gitlab.com/gitlab-org/gitlab-pages/internal/httputil" ) func endsWithSlash(path string) bool { -- GitLab From b37ed1c0ff9880fb73ed2ed771c100333d79f3c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 1 Oct 2019 11:53:16 +0200 Subject: [PATCH 14/17] Refactor the code to use `Serving/LookupPath` This moves the code from the concept of `Project` to use a concept of `LookupPath`. This makes the `LookupPath` to define a `Path` on disk that the data is being served from. This makes the `ACME` to not have special handling, but rather try to serve the file if succeeds, skip GitLab passthrough for ACME. --- internal/acme/acme.go | 4 +- internal/acme/acme_test.go | 8 +++- internal/domain/domain.go | 41 +++++++----------- internal/domain/domain_test.go | 38 +++++++++-------- internal/domain/handler.go | 42 ------------------- internal/domain/resolver.go | 8 +++- internal/serving/disk.go | 19 --------- internal/serving/disk/handler.go | 11 ----- internal/serving/disk/reader.go | 27 ++++++------ internal/serving/disk/serving.go | 28 ++++--------- internal/serving/handler.go | 20 ++++----- .../project.go => serving/lookup_path.go} | 7 ++-- internal/serving/serving.go | 16 ------- internal/source/disk/custom.go | 11 +++-- internal/source/disk/domain_test.go | 36 +++++----------- internal/source/disk/group.go | 21 ++++++---- internal/source/disk/map.go | 22 +++++----- 17 files changed, 126 insertions(+), 233 deletions(-) delete mode 100644 internal/domain/handler.go delete mode 100644 internal/serving/disk.go delete mode 100644 internal/serving/disk/handler.go rename internal/{domain/project.go => serving/lookup_path.go} (62%) diff --git a/internal/acme/acme.go b/internal/acme/acme.go index a6be01dec..3bfa8f2e0 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -18,7 +18,7 @@ type Middleware struct { // Domain interface represent D from domain package type Domain interface { - HasAcmeChallenge(*http.Request, string) bool + ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool } // ServeAcmeChallenges identifies if request is acme-challenge and redirects to GitLab in that case @@ -31,7 +31,7 @@ func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, return false } - if domain.HasAcmeChallenge(r, filepath.Base(r.URL.Path)) { + if domain.ServeFileHTTP(w, r) { return false } diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go index 00932d3e9..ab191694f 100644 --- a/internal/acme/acme_test.go +++ b/internal/acme/acme_test.go @@ -11,8 +11,12 @@ type domainStub struct { hasAcmeChallenge bool } -func (d *domainStub) HasAcmeChallenge(_ *http.Request, _ string) bool { - return d.hasAcmeChallenge +func (d *domainStub) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { + if r.URL.Path == "/.well-known/acme-challenge/token" { + return d.hasAcmeChallenge + } + + return false } func serveAcmeOrNotFound(m *Middleware, domain Domain) http.HandlerFunc { diff --git a/internal/domain/domain.go b/internal/domain/domain.go index df484dd2c..2941d1743 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -8,19 +8,18 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" ) // Domain is a domain that gitlab-pages can serve. type Domain struct { Name string - Location string CertificateCert string CertificateKey string Resolver Resolver - lookupPaths map[string]*Project - serving serving.Serving + serving serving.Serving certificate *tls.Certificate certificateError error @@ -40,7 +39,7 @@ func (d *Domain) isUnconfigured() bool { return d.Resolver == nil } -func (d *Domain) resolve(r *http.Request) (*Project, string) { +func (d *Domain) resolve(r *http.Request) (*serving.LookupPath, string) { // TODO use lookupPaths to cache information about projects better, to // improve performance and resilience @@ -55,7 +54,7 @@ func (d *Domain) resolve(r *http.Request) (*Project, string) { } // GetProject returns a project details based on the request -func (d *Domain) GetProject(r *http.Request) *Project { +func (d *Domain) GetProject(r *http.Request) *serving.LookupPath { project, _ := d.resolve(r) return project @@ -64,20 +63,20 @@ func (d *Domain) GetProject(r *http.Request) *Project { // Serving returns domain serving driver func (d *Domain) Serving() serving.Serving { if d.serving == nil { - d.serving = serving.NewDiskServing(d.Name, d.Location) + d.serving = disk.New() } return d.serving } -func (d *Domain) toHandler(w http.ResponseWriter, r *http.Request) *handler { +func (d *Domain) toHandler(w http.ResponseWriter, r *http.Request) serving.Handler { project, subpath := d.resolve(r) - return &handler{ - writer: w, - request: r, - project: project, - subpath: subpath, + return serving.Handler{ + Writer: w, + Request: r, + LookupPath: project, + SubPath: subpath, } } @@ -108,20 +107,6 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { return false } -// HasAcmeChallenge checks domain directory contains particular acme challenge -func (d *Domain) HasAcmeChallenge(r *http.Request, token string) bool { - // TODO is that safe to redirect to acme challenge in GitLab if it is a grup domain? - if d.isUnconfigured() || !d.HasProject(r) { - return false - } - - // TODO we should improve that, we need different type of information to - // check if the ACME challenge is present in the serving. We should devise a - // better interface here or we should to extract this responsibility - // somewhere else. - return d.Serving().HasAcmeChallenge(d.toHandler(nil, r), token) -} - // IsNamespaceProject figures out if the request is to a namespace project func (d *Domain) IsNamespaceProject(r *http.Request) bool { if d.isUnconfigured() { @@ -180,6 +165,10 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { // ServeFileHTTP returns true if something was served, false if not. func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { if d.isUnconfigured() || !d.HasProject(r) { + // TODO: this seems to be wrong: + // as we should rather return false, + // and fallback to `ServeNotFoundHTTP` + // to handle this case httperrors.Serve404(w) return true } diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 8b3690754..a43d3c9a9 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -11,16 +11,17 @@ import ( "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) type stubbedResolver struct { - project *Project + project *serving.LookupPath subpath string err error } -func (resolver *stubbedResolver) Resolve(*http.Request) (*Project, string, error) { +func (resolver *stubbedResolver) Resolve(*http.Request) (*serving.LookupPath, string, error) { return resolver.project, resolver.subpath, resolver.err } @@ -42,8 +43,12 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only enabled", domain: &Domain{ - Location: "group/project", - Resolver: &stubbedResolver{project: &Project{IsHTTPSOnly: true}}, + Resolver: &stubbedResolver{ + project: &serving.LookupPath{ + Path: "group/project/public", + IsHTTPSOnly: true, + }, + }, }, url: "http://custom-domain", expected: true, @@ -51,17 +56,19 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only disabled", domain: &Domain{ - Location: "group/project", - Resolver: &stubbedResolver{project: &Project{IsHTTPSOnly: false}}, + Resolver: &stubbedResolver{ + project: &serving.LookupPath{ + Path: "group/project/public", + IsHTTPSOnly: false, + }, + }, }, url: "http://custom-domain", expected: false, }, { - name: "Unknown project", - domain: &Domain{ - Location: "group/project", - }, + name: "Unknown project", + domain: &Domain{}, url: "http://test-domain/project", expected: false, }, @@ -106,17 +113,13 @@ func TestPredefined404ServeHTTP(t *testing.T) { cleanup := setUpTests(t) defer cleanup() - testDomain := &Domain{ - Location: "group", - } + testDomain := &Domain{} testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found") } func TestGroupCertificate(t *testing.T) { - testGroup := &Domain{ - Location: "group", - } + testGroup := &Domain{} tls, err := testGroup.EnsureCertificate() require.Nil(t, tls) @@ -125,8 +128,7 @@ func TestGroupCertificate(t *testing.T) { func TestDomainNoCertificate(t *testing.T) { testDomain := &Domain{ - Name: "test.domain.com", - Location: "group/project2", + Name: "test.domain.com", } tls, err := testDomain.EnsureCertificate() diff --git a/internal/domain/handler.go b/internal/domain/handler.go deleted file mode 100644 index 55697700d..000000000 --- a/internal/domain/handler.go +++ /dev/null @@ -1,42 +0,0 @@ -package domain - -import "net/http" - -type handler struct { - writer http.ResponseWriter - request *http.Request - project *Project - subpath string -} - -func (h *handler) Writer() http.ResponseWriter { - return h.writer -} - -func (h *handler) Request() *http.Request { - return h.request -} - -func (h *handler) LookupPath() string { - return h.project.LookupPath -} - -func (h *handler) Subpath() string { - return h.subpath -} - -func (h *handler) IsNamespaceProject() bool { - return h.project.IsNamespaceProject -} - -func (h *handler) IsHTTPSOnly() bool { - return h.project.IsHTTPSOnly -} - -func (h *handler) HasAccessControl() bool { - return h.project.HasAccessControl -} - -func (h *handler) ProjectID() uint64 { - return h.project.ID -} diff --git a/internal/domain/resolver.go b/internal/domain/resolver.go index 5bde31ec2..0de66ad59 100644 --- a/internal/domain/resolver.go +++ b/internal/domain/resolver.go @@ -1,10 +1,14 @@ package domain -import "net/http" +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" +) // Resolver represents an interface responsible for resolving a project // per-request type Resolver interface { // Resolve returns a project with a file path and an error if it occured - Resolve(*http.Request) (*Project, string, error) + Resolve(*http.Request) (*serving.LookupPath, string, error) } diff --git a/internal/serving/disk.go b/internal/serving/disk.go deleted file mode 100644 index b569986fe..000000000 --- a/internal/serving/disk.go +++ /dev/null @@ -1,19 +0,0 @@ -package serving - -import "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" - -type diskServing struct { - disk *disk.Serving -} - -func (d *diskServing) ServeFileHTTP(h Handler) bool { - return d.disk.ServeFileHTTP(h) -} - -func (d *diskServing) ServeNotFoundHTTP(h Handler) { - d.disk.ServeNotFoundHTTP(h) -} - -func (d *diskServing) HasAcmeChallenge(h Handler, token string) bool { - return d.disk.HasAcmeChallenge(h, token) -} diff --git a/internal/serving/disk/handler.go b/internal/serving/disk/handler.go deleted file mode 100644 index fbbf9ce3b..000000000 --- a/internal/serving/disk/handler.go +++ /dev/null @@ -1,11 +0,0 @@ -package disk - -import "net/http" - -type handler interface { - Writer() http.ResponseWriter - Request() *http.Request - LookupPath() string - Subpath() string - HasAccessControl() bool -} diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index 1fc30da7d..b52b5cffe 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -9,23 +9,24 @@ import ( "strconv" "strings" "time" + + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" ) // Reader is a disk access driver type Reader struct { - Location string } -func (reader *Reader) tryFile(h handler) error { - fullPath, err := reader.resolvePath(h.LookupPath(), h.Subpath()) +func (reader *Reader) tryFile(h serving.Handler) error { + fullPath, err := reader.resolvePath(h.LookupPath.Path, h.SubPath) - request := h.Request() + request := h.Request host := request.Host urlPath := request.URL.Path if locationError, _ := err.(*locationDirectoryError); locationError != nil { if endsWithSlash(urlPath) { - fullPath, err = reader.resolvePath(h.LookupPath(), h.Subpath(), "index.html") + fullPath, err = reader.resolvePath(h.LookupPath.Path, h.SubPath, "index.html") } else { // Concat Host with URL.Path redirectPath := "//" + host + "/" @@ -33,29 +34,29 @@ func (reader *Reader) tryFile(h handler) error { // Ensure that there's always "/" at end redirectPath = strings.TrimSuffix(redirectPath, "/") + "/" - http.Redirect(h.Writer(), h.Request(), redirectPath, 302) + http.Redirect(h.Writer, h.Request, redirectPath, 302) return nil } } if locationError, _ := err.(*locationFileNoExtensionError); locationError != nil { - fullPath, err = reader.resolvePath(h.LookupPath(), strings.TrimSuffix(h.Subpath(), "/")+".html") + fullPath, err = reader.resolvePath(h.LookupPath.Path, strings.TrimSuffix(h.SubPath, "/")+".html") } if err != nil { return err } - return reader.serveFile(h.Writer(), h.Request(), fullPath, h.HasAccessControl()) + return reader.serveFile(h.Writer, h.Request, fullPath, h.LookupPath.HasAccessControl) } -func (reader *Reader) tryNotFound(h handler) error { - page404, err := reader.resolvePath(h.LookupPath(), "404.html") +func (reader *Reader) tryNotFound(h serving.Handler) error { + page404, err := reader.resolvePath(h.LookupPath.Path, "404.html") if err != nil { return err } - err = reader.serveCustomFile(h.Writer(), h.Request(), http.StatusNotFound, page404) + err = reader.serveCustomFile(h.Writer, h.Request, http.StatusNotFound, page404) if err != nil { return err } @@ -64,9 +65,7 @@ func (reader *Reader) tryNotFound(h handler) error { // Resolve the HTTP request to a path on disk, converting requests for // directories to requests for index.html inside the directory if appropriate. -func (reader *Reader) resolvePath(lookupPath string, subPath ...string) (string, error) { - publicPath := filepath.Join(reader.Location, lookupPath, "public") - +func (reader *Reader) resolvePath(publicPath string, subPath ...string) (string, error) { // Don't use filepath.Join as cleans the path, // where we want to traverse full path as supplied by user // (including ..) diff --git a/internal/serving/disk/serving.go b/internal/serving/disk/serving.go index 78b1b5721..db184d3cc 100644 --- a/internal/serving/disk/serving.go +++ b/internal/serving/disk/serving.go @@ -2,17 +2,17 @@ package disk import ( "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" ) // Serving describes a disk access serving type Serving struct { - Domain string // TODO it is not used but might be handy - *Reader + Reader } // ServeFileHTTP serves a file from disk and returns true. It returns false // when a file could not been found. -func (s *Serving) ServeFileHTTP(h handler) bool { +func (s *Serving) ServeFileHTTP(h serving.Handler) bool { if s.tryFile(h) == nil { return true } @@ -21,27 +21,17 @@ func (s *Serving) ServeFileHTTP(h handler) bool { } // ServeNotFoundHTTP tries to read a custom 404 page -func (s *Serving) ServeNotFoundHTTP(h handler) { +func (s *Serving) ServeNotFoundHTTP(h serving.Handler) { if s.tryNotFound(h) == nil { return } // Generic 404 - httperrors.Serve404(h.Writer()) + httperrors.Serve404(h.Writer) } -// HasAcmeChallenge checks if the ACME challenge is present on the disk -func (s *Serving) HasAcmeChallenge(h handler, token string) bool { - _, err := s.resolvePath(h.LookupPath(), ".well-known/acme-challenge", token) - // there is an acme challenge on disk - if err == nil { - return true - } - - _, err = s.resolvePath(h.LookupPath(), ".well-known/acme-challenge", token, "index.html") - if err == nil { - return true - } - - return false +// New returns a serving instance that is capable of reading files +// from the disk +func New() serving.Serving { + return &Serving{} } diff --git a/internal/serving/handler.go b/internal/serving/handler.go index 2a9969b7b..446c98432 100644 --- a/internal/serving/handler.go +++ b/internal/serving/handler.go @@ -2,15 +2,13 @@ package serving import "net/http" -// Handler interface represent an interface that is needed to fullfil the -// serving request -type Handler interface { - Writer() http.ResponseWriter - Request() *http.Request - LookupPath() string - Subpath() string - IsNamespaceProject() bool - IsHTTPSOnly() bool - HasAccessControl() bool - ProjectID() uint64 +type Handler struct { + Writer http.ResponseWriter + Request *http.Request + + LookupPath *LookupPath + + // parsed representation of Request.URI + // that is part of LookupPath.Prefix + SubPath string } diff --git a/internal/domain/project.go b/internal/serving/lookup_path.go similarity index 62% rename from internal/domain/project.go rename to internal/serving/lookup_path.go index 9ea7306f8..77ac45e8d 100644 --- a/internal/domain/project.go +++ b/internal/serving/lookup_path.go @@ -1,8 +1,9 @@ -package domain +package serving // Project holds a domain / project configuration -type Project struct { - LookupPath string +type LookupPath struct { + Location string + Path string IsNamespaceProject bool IsHTTPSOnly bool HasAccessControl bool diff --git a/internal/serving/serving.go b/internal/serving/serving.go index c6a392411..6fde82165 100644 --- a/internal/serving/serving.go +++ b/internal/serving/serving.go @@ -1,23 +1,7 @@ package serving -import ( - "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" -) - // Serving is an interface used to define a serving driver type Serving interface { ServeFileHTTP(Handler) bool ServeNotFoundHTTP(Handler) - HasAcmeChallenge(handler Handler, token string) bool -} - -// NewDiskServing returns a serving instance that is capable of reading files -// from the disk -func NewDiskServing(domain, location string) Serving { - return &diskServing{ - disk: &disk.Serving{ - Domain: domain, - Reader: &disk.Reader{Location: location}, - }, - } } diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go index 0cf443b0c..a7c89700c 100644 --- a/internal/source/disk/custom.go +++ b/internal/source/disk/custom.go @@ -3,17 +3,20 @@ package disk import ( "net/http" - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" ) type customProjectResolver struct { config *domainConfig + + path string } // TODO tests -func (p *customProjectResolver) Resolve(r *http.Request) (*domain.Project, string, error) { - project := &domain.Project{ - LookupPath: "/", +func (p *customProjectResolver) Resolve(r *http.Request) (*serving.LookupPath, string, error) { + project := &serving.LookupPath{ + Location: "/", + Path: p.path, IsNamespaceProject: false, IsHTTPSOnly: p.config.HTTPSOnly, HasAccessControl: p.config.AccessControl, diff --git a/internal/source/disk/domain_test.go b/internal/source/disk/domain_test.go index 4140241e6..ba4fb1618 100644 --- a/internal/source/disk/domain_test.go +++ b/internal/source/disk/domain_test.go @@ -27,7 +27,6 @@ func serveFileOrNotFound(domain *domain.Domain) http.HandlerFunc { func testGroupServeHTTPHost(t *testing.T, host string) { testGroup := &domain.Domain{ - Location: "group", Resolver: &Group{ name: "group", projects: map[string]*projectConfig{ @@ -81,9 +80,9 @@ func TestDomainServeHTTP(t *testing.T) { defer cleanup() testDomain := &domain.Domain{ - Name: "test.domain.com", - Location: "group/project2", + Name: "test.domain.com", Resolver: &customProjectResolver{ + path: "group/project2/public", config: &domainConfig{}, }, } @@ -109,7 +108,6 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Default group domain with HTTPS-only enabled", domain: &domain.Domain{ - Location: "group/project", Resolver: &Group{ name: "group", projects: projects{"test-domain": &projectConfig{HTTPSOnly: true}}, @@ -121,7 +119,6 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Default group domain with HTTPS-only disabled", domain: &domain.Domain{ - Location: "group/project", Resolver: &Group{ name: "group", projects: projects{"test-domain": &projectConfig{HTTPSOnly: false}}, @@ -133,7 +130,6 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Case-insensitive default group domain with HTTPS-only enabled", domain: &domain.Domain{ - Location: "group/project", Resolver: &Group{ name: "group", projects: projects{"test-domain": &projectConfig{HTTPSOnly: true}}, @@ -145,7 +141,6 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Other group domain with HTTPS-only enabled", domain: &domain.Domain{ - Location: "group/project", Resolver: &Group{ name: "group", projects: projects{"project": &projectConfig{HTTPSOnly: true}}, @@ -157,7 +152,6 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Other group domain with HTTPS-only disabled", domain: &domain.Domain{ - Location: "group/project", Resolver: &Group{ name: "group", projects: projects{"project": &projectConfig{HTTPSOnly: false}}, @@ -167,10 +161,8 @@ func TestIsHTTPSOnly(t *testing.T) { expected: false, }, { - name: "Unknown project", - domain: &domain.Domain{ - Location: "group/project", - }, + name: "Unknown project", + domain: &domain.Domain{}, url: "http://test-domain/project", expected: false, }, @@ -216,7 +208,6 @@ func TestGroupServeHTTPGzip(t *testing.T) { defer cleanup() testGroup := &domain.Domain{ - Location: "group", Resolver: &Group{ name: "group", projects: map[string]*projectConfig{ @@ -283,7 +274,6 @@ func TestGroup404ServeHTTP(t *testing.T) { defer cleanup() testGroup := &domain.Domain{ - Location: "group.404", Resolver: &Group{ name: "group.404", projects: map[string]*projectConfig{ @@ -312,8 +302,8 @@ func TestDomain404ServeHTTP(t *testing.T) { defer cleanup() testDomain := &domain.Domain{ - Location: "group.404/domain.404", Resolver: &customProjectResolver{ + path: "group.404/domain.404/public", config: &domainConfig{Domain: "domain.404.com"}, }, } @@ -326,17 +316,13 @@ func TestPredefined404ServeHTTP(t *testing.T) { cleanup := setUpTests(t) defer cleanup() - testDomain := &domain.Domain{ - Location: "group", - } + testDomain := &domain.Domain{} testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found") } func TestGroupCertificate(t *testing.T) { - testGroup := &domain.Domain{ - Location: "group", - } + testGroup := &domain.Domain{} tls, err := testGroup.EnsureCertificate() require.Nil(t, tls) @@ -345,8 +331,8 @@ func TestGroupCertificate(t *testing.T) { func TestDomainNoCertificate(t *testing.T) { testDomain := &domain.Domain{ - Location: "group/project2", Resolver: &customProjectResolver{ + path: "group/project2/public", config: &domainConfig{Domain: "test.domain.com"}, }, } @@ -362,11 +348,12 @@ func TestDomainNoCertificate(t *testing.T) { func TestDomainCertificate(t *testing.T) { testDomain := &domain.Domain{ - Location: "group/project2", Name: "test.domain.com", CertificateCert: fixture.Certificate, CertificateKey: fixture.Key, - Resolver: &customProjectResolver{}, + Resolver: &customProjectResolver{ + path: "group/project2/public", + }, } tls, err := testDomain.EnsureCertificate() @@ -379,7 +366,6 @@ func TestCacheControlHeaders(t *testing.T) { defer cleanup() testGroup := &domain.Domain{ - Location: "group", Resolver: &Group{ name: "group", projects: map[string]*projectConfig{ diff --git a/internal/source/disk/group.go b/internal/source/disk/group.go index efa3fce5b..df0939260 100644 --- a/internal/source/disk/group.go +++ b/internal/source/disk/group.go @@ -3,10 +3,11 @@ package disk import ( "net/http" "path" + "path/filepath" "strings" - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/host" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" ) const ( @@ -52,14 +53,14 @@ func (g *Group) digProjectWithSubpath(parentPath string, keys []string) (*projec // Look up a project inside the domain based on the host and path. Returns the // project and its name (if applicable) -func (g *Group) getProjectConfigWithSubpath(r *http.Request) (*projectConfig, string, string) { +func (g *Group) getProjectConfigWithSubpath(r *http.Request) (*projectConfig, string, string, string) { // Check for a project specified in the URL: http://group.gitlab.io/projectA // If present, these projects shadow the group domain. split := strings.SplitN(r.URL.Path, "/", maxProjectDepth) if len(split) >= 2 { projectConfig, projectPath, urlPath := g.digProjectWithSubpath("", split[1:]) if projectConfig != nil { - return projectConfig, projectPath, urlPath + return projectConfig, "/" + projectPath, projectPath, urlPath } } @@ -67,24 +68,26 @@ func (g *Group) getProjectConfigWithSubpath(r *http.Request) (*projectConfig, st // return the group project if it exists. if host := host.FromRequest(r); host != "" { if groupProject := g.projects[host]; groupProject != nil { - return groupProject, host, strings.Join(split[1:], "/") + // TODOHERE: the location here should be "/", so we return "" + return groupProject, "/", host, strings.Join(split[1:], "/") } } - return nil, "", "" + return nil, "", "", "" } // Resolve tries to find project and its config recursively for a given request // to a group domain -func (g *Group) Resolve(r *http.Request) (*domain.Project, string, error) { - projectConfig, projectPath, subPath := g.getProjectConfigWithSubpath(r) +func (g *Group) Resolve(r *http.Request) (*serving.LookupPath, string, error) { + projectConfig, location, projectPath, subPath := g.getProjectConfigWithSubpath(r) if projectConfig == nil { return nil, "", nil // it is not an error when project does not exist } - project := &domain.Project{ - LookupPath: projectPath, + project := &serving.LookupPath{ + Location: location, + Path: filepath.Join(g.name, projectPath, "public"), IsNamespaceProject: projectConfig.NamespaceProject, IsHTTPSOnly: projectConfig.HTTPSOnly, HasAccessControl: projectConfig.AccessControl, diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index 09902241f..3fb7bd4a5 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -22,13 +22,14 @@ type Map map[string]*domain.Domain type domainsUpdater func(Map) func (dm Map) updateDomainMap(domainName string, domain *domain.Domain) { - if old, ok := dm[domainName]; ok { - log.WithFields(log.Fields{ - "domain_name": domainName, - "new_location": domain.Location, - "old_location": old.Location, - }).Error("Duplicate domain") - } + // TODOHERE: + // if old, ok := dm[domainName]; ok { + // log.WithFields(log.Fields{ + // "domain_name": domainName, + // "new_location": domain.Location, + // "old_location": old.Location, + // }).Error("Duplicate domain") + // } dm[domainName] = domain } @@ -38,8 +39,10 @@ func (dm Map) addDomain(rootDomain, groupName, projectName string, config *domai Name: strings.ToLower(config.Domain), CertificateCert: config.Certificate, CertificateKey: config.Key, - Location: filepath.Join(groupName, projectName), - Resolver: &customProjectResolver{config: config}, + Resolver: &customProjectResolver{ + config: config, + path: filepath.Join(groupName, projectName, "public"), + }, } dm.updateDomainMap(newDomain.Name, newDomain) @@ -58,7 +61,6 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https groupDomain = &domain.Domain{ Name: domainName, - Location: groupName, Resolver: groupResolver, } } -- GitLab From 75111473589791e1d11dc9225798a19d248a6cbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 1 Oct 2019 12:00:52 +0200 Subject: [PATCH 15/17] Remove references to `Project` Always uses `LookupPath`. The `project` is local to `source/disk`. --- app.go | 4 +-- internal/domain/domain.go | 44 ++++++++++++++++----------------- internal/logging/logging.go | 2 +- internal/serving/lookup_path.go | 2 +- internal/source/disk/custom.go | 6 ++--- internal/source/disk/group.go | 6 ++--- 6 files changed, 32 insertions(+), 32 deletions(-) diff --git a/app.go b/app.go index 67b94f2ad..82cc2680d 100644 --- a/app.go +++ b/app.go @@ -101,7 +101,7 @@ func (a *theApp) domain(host string) *domain.Domain { } func (a *theApp) checkAuthenticationIfNotExists(domain *domain.Domain, w http.ResponseWriter, r *http.Request) bool { - if domain == nil || !domain.HasProject(r) { + if domain == nil || !domain.HasLookupPath(r) { // Only if auth is supported if a.Auth.IsAuthSupported() { @@ -231,7 +231,7 @@ func (a *theApp) accessControlMiddleware(handler http.Handler) http.Handler { // Only for projects that have access control enabled if domain.IsAccessControlEnabled(r) { // accessControlMiddleware - if a.Auth.CheckAuthentication(w, r, domain.GetID(r)) { + if a.Auth.CheckAuthentication(w, r, domain.GetProjectID(r)) { return } } diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 2941d1743..febb488fe 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -43,21 +43,21 @@ func (d *Domain) resolve(r *http.Request) (*serving.LookupPath, string) { // TODO use lookupPaths to cache information about projects better, to // improve performance and resilience - project, subpath, _ := d.Resolver.Resolve(r) + lookupPath, subpath, _ := d.Resolver.Resolve(r) // Current implementation does not return errors in any case - if project == nil { + if lookupPath == nil { return nil, "" } - return project, subpath + return lookupPath, subpath } -// GetProject returns a project details based on the request -func (d *Domain) GetProject(r *http.Request) *serving.LookupPath { - project, _ := d.resolve(r) +// GetLookupPath returns a project details based on the request +func (d *Domain) GetLookupPath(r *http.Request) *serving.LookupPath { + lookupPath, _ := d.resolve(r) - return project + return lookupPath } // Serving returns domain serving driver @@ -87,8 +87,8 @@ func (d *Domain) IsHTTPSOnly(r *http.Request) bool { return false } - if project := d.GetProject(r); project != nil { - return project.IsHTTPSOnly + if lookupPath := d.GetLookupPath(r); lookupPath != nil { + return lookupPath.IsHTTPSOnly } return false @@ -100,8 +100,8 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { return false } - if project := d.GetProject(r); project != nil { - return project.HasAccessControl + if lookupPath := d.GetLookupPath(r); lookupPath != nil { + return lookupPath.HasAccessControl } return false @@ -113,33 +113,33 @@ func (d *Domain) IsNamespaceProject(r *http.Request) bool { return false } - if project := d.GetProject(r); project != nil { - return project.IsNamespaceProject + if lookupPath := d.GetLookupPath(r); lookupPath != nil { + return lookupPath.IsNamespaceProject } return false } -// GetID figures out what is the ID of the project user tries to access -func (d *Domain) GetID(r *http.Request) uint64 { +// GetProjectID figures out what is the ID of the project user tries to access +func (d *Domain) GetProjectID(r *http.Request) uint64 { if d.isUnconfigured() { return 0 } - if project := d.GetProject(r); project != nil { - return project.ID + if lookupPath := d.GetLookupPath(r); lookupPath != nil { + return lookupPath.ProjectID } return 0 } -// HasProject figures out if the project exists that the user tries to access -func (d *Domain) HasProject(r *http.Request) bool { +// HasLookupPath figures out if the project exists that the user tries to access +func (d *Domain) HasLookupPath(r *http.Request) bool { if d.isUnconfigured() { return false } - return d.GetProject(r) != nil + return d.GetLookupPath(r) != nil } // EnsureCertificate parses the PEM-encoded certificate for the domain @@ -164,7 +164,7 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { // ServeFileHTTP returns true if something was served, false if not. func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { - if d.isUnconfigured() || !d.HasProject(r) { + if d.isUnconfigured() || !d.HasLookupPath(r) { // TODO: this seems to be wrong: // as we should rather return false, // and fallback to `ServeNotFoundHTTP` @@ -178,7 +178,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { // ServeNotFoundHTTP serves the not found pages from the projects. func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { - if d.isUnconfigured() || !d.HasProject(r) { + if d.isUnconfigured() || !d.HasLookupPath(r) { httperrors.Serve404(w) return } diff --git a/internal/logging/logging.go b/internal/logging/logging.go index f0682573c..7c2f013e4 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -55,7 +55,7 @@ func getAccessLogger(format string) (*logrus.Logger, error) { func getExtraLogFields(r *http.Request) log.Fields { var projectID uint64 if d := request.GetDomain(r); d != nil { - projectID = d.GetID(r) + projectID = d.GetProjectID(r) } return log.Fields{ diff --git a/internal/serving/lookup_path.go b/internal/serving/lookup_path.go index 77ac45e8d..0b651d53e 100644 --- a/internal/serving/lookup_path.go +++ b/internal/serving/lookup_path.go @@ -7,5 +7,5 @@ type LookupPath struct { IsNamespaceProject bool IsHTTPSOnly bool HasAccessControl bool - ID uint64 + ProjectID uint64 } diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go index a7c89700c..8a080f208 100644 --- a/internal/source/disk/custom.go +++ b/internal/source/disk/custom.go @@ -14,14 +14,14 @@ type customProjectResolver struct { // TODO tests func (p *customProjectResolver) Resolve(r *http.Request) (*serving.LookupPath, string, error) { - project := &serving.LookupPath{ + lookupPath := &serving.LookupPath{ Location: "/", Path: p.path, IsNamespaceProject: false, IsHTTPSOnly: p.config.HTTPSOnly, HasAccessControl: p.config.AccessControl, - ID: p.config.ID, + ProjectID: p.config.ID, } - return project, r.URL.Path, nil + return lookupPath, r.URL.Path, nil } diff --git a/internal/source/disk/group.go b/internal/source/disk/group.go index df0939260..59aedc739 100644 --- a/internal/source/disk/group.go +++ b/internal/source/disk/group.go @@ -85,14 +85,14 @@ func (g *Group) Resolve(r *http.Request) (*serving.LookupPath, string, error) { return nil, "", nil // it is not an error when project does not exist } - project := &serving.LookupPath{ + lookupPath := &serving.LookupPath{ Location: location, Path: filepath.Join(g.name, projectPath, "public"), IsNamespaceProject: projectConfig.NamespaceProject, IsHTTPSOnly: projectConfig.HTTPSOnly, HasAccessControl: projectConfig.AccessControl, - ID: projectConfig.ID, + ProjectID: projectConfig.ID, } - return project, subPath, nil + return lookupPath, subPath, nil } -- GitLab From ffe374c1fc0761f4899783cc81149c530d58ef00 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 1 Oct 2019 12:28:10 +0200 Subject: [PATCH 16/17] Fix golint warnings in hadnler / lookup path --- internal/serving/handler.go | 11 +++++------ internal/serving/lookup_path.go | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/serving/handler.go b/internal/serving/handler.go index 446c98432..53a783cef 100644 --- a/internal/serving/handler.go +++ b/internal/serving/handler.go @@ -2,13 +2,12 @@ package serving import "net/http" +// Handler agregates response/request and lookup path + subpath needed to +// handle a request and response. type Handler struct { - Writer http.ResponseWriter - Request *http.Request - + Writer http.ResponseWriter + Request *http.Request LookupPath *LookupPath - - // parsed representation of Request.URI - // that is part of LookupPath.Prefix + // Parsed representation of Request.URI that is part of LookupPath.Prefix SubPath string } diff --git a/internal/serving/lookup_path.go b/internal/serving/lookup_path.go index 0b651d53e..ba6e8f7a9 100644 --- a/internal/serving/lookup_path.go +++ b/internal/serving/lookup_path.go @@ -1,6 +1,6 @@ package serving -// Project holds a domain / project configuration +// LookupPath holds a domain project configuration needed to handle a request type LookupPath struct { Location string Path string -- GitLab From 78c7e7be19993403054f8584fbc5b6c17885b20f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 2 Oct 2019 09:32:46 +0200 Subject: [PATCH 17/17] Only log domain name as we no longer have location info --- go.mod | 2 -- internal/domain/domain.go | 3 --- internal/source/disk/map.go | 13 +++++-------- 3 files changed, 5 insertions(+), 13 deletions(-) diff --git a/go.mod b/go.mod index 046398e28..2c4252a45 100644 --- a/go.mod +++ b/go.mod @@ -13,8 +13,6 @@ require ( github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 github.com/karrick/godirwalk v1.10.12 - github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect - github.com/modern-go/reflect2 v1.0.1 // indirect github.com/namsral/flag v1.7.4-pre github.com/prometheus/client_golang v1.1.0 github.com/rs/cors v1.7.0 diff --git a/internal/domain/domain.go b/internal/domain/domain.go index febb488fe..f7eba5cad 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -40,9 +40,6 @@ func (d *Domain) isUnconfigured() bool { } func (d *Domain) resolve(r *http.Request) (*serving.LookupPath, string) { - // TODO use lookupPaths to cache information about projects better, to - // improve performance and resilience - lookupPath, subpath, _ := d.Resolver.Resolve(r) // Current implementation does not return errors in any case diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index 3fb7bd4a5..b58433019 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -22,14 +22,11 @@ type Map map[string]*domain.Domain type domainsUpdater func(Map) func (dm Map) updateDomainMap(domainName string, domain *domain.Domain) { - // TODOHERE: - // if old, ok := dm[domainName]; ok { - // log.WithFields(log.Fields{ - // "domain_name": domainName, - // "new_location": domain.Location, - // "old_location": old.Location, - // }).Error("Duplicate domain") - // } + if _, ok := dm[domainName]; ok { + log.WithFields(log.Fields{ + "domain_name": domainName, + }).Error("Duplicate domain") + } dm[domainName] = domain } -- GitLab