From c5ea1f1db89bc5bb53714b071ebbd167c28ab0e7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 20 Sep 2019 13:56:35 +0200 Subject: [PATCH 01/13] Make it more explicit what a Group and Project is --- go.mod | 2 + internal/domain/domain.go | 15 ++---- internal/domain/domain_test.go | 90 +++++++++++++++++----------------- internal/domain/group.go | 8 +-- internal/domain/group_test.go | 30 ++++++------ internal/domain/map.go | 8 +-- internal/domain/map_test.go | 2 +- internal/domain/project.go | 8 +++ 8 files changed, 83 insertions(+), 80 deletions(-) create mode 100644 internal/domain/project.go 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/domain/domain.go b/internal/domain/domain.go index 2f499ff97..efea118a3 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -38,16 +38,9 @@ type locationFileNoExtensionError struct { FullPath string } -type project struct { - NamespaceProject bool - HTTPSOnly bool - AccessControl bool - ID uint64 -} - // Domain is a domain that gitlab-pages can serve. type Domain struct { - group + group Group // custom domains: projectName string @@ -108,12 +101,12 @@ func handleGZip(w http.ResponseWriter, r *http.Request, fullPath string) string // Look up a project inside the domain based on the host and path. Returns the // project and its name (if applicable) -func (d *Domain) getProjectWithSubpath(r *http.Request) (*project, string, string) { +func (d *Domain) getProjectWithSubpath(r *http.Request) (*Project, 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 { - project, projectPath, urlPath := d.digProjectWithSubpath("", split[1:]) + project, projectPath, urlPath := d.group.digProjectWithSubpath("", split[1:]) if project != nil { return project, projectPath, urlPath } @@ -122,7 +115,7 @@ func (d *Domain) getProjectWithSubpath(r *http.Request) (*project, string, strin // Since the URL doesn't specify a project (e.g. http://mydomain.gitlab.io), // return the group project if it exists. if host := host.FromRequest(r); host != "" { - if groupProject := d.projects[host]; groupProject != nil { + if groupProject := d.group.projects[host]; groupProject != nil { return groupProject, host, strings.Join(split[1:], "/") } } diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index d5db33c96..1f200afae 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -27,13 +27,13 @@ func serveFileOrNotFound(domain *Domain) http.HandlerFunc { func testGroupServeHTTPHost(t *testing.T, host string) { testGroup := &Domain{ projectName: "", - group: group{ + group: Group{ name: "group", - projects: map[string]*project{ - "group.test.io": &project{}, - "group.gitlab-example.com": &project{}, - "project": &project{}, - "project2": &project{}, + projects: map[string]*Project{ + "group.test.io": &Project{}, + "group.gitlab-example.com": &Project{}, + "project": &Project{}, + "project2": &Project{}, }, }, } @@ -80,7 +80,7 @@ func TestDomainServeHTTP(t *testing.T) { defer cleanup() testDomain := &Domain{ - group: group{name: "group"}, + group: Group{name: "group"}, projectName: "project2", config: &domainConfig{ Domain: "test.domain.com", @@ -108,7 +108,7 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only enabled", domain: &Domain{ - group: group{name: "group"}, + group: Group{name: "group"}, projectName: "project", config: &domainConfig{HTTPSOnly: true}, }, @@ -118,7 +118,7 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only disabled", domain: &Domain{ - group: group{name: "group"}, + group: Group{name: "group"}, projectName: "project", config: &domainConfig{HTTPSOnly: false}, }, @@ -129,9 +129,9 @@ func TestIsHTTPSOnly(t *testing.T) { name: "Default group domain with HTTPS-only enabled", domain: &Domain{ projectName: "project", - group: group{ + group: Group{ name: "group", - projects: projects{"test-domain": &project{HTTPSOnly: true}}, + projects: projects{"test-domain": &Project{HTTPSOnly: true}}, }, }, url: "http://test-domain", @@ -141,9 +141,9 @@ func TestIsHTTPSOnly(t *testing.T) { name: "Default group domain with HTTPS-only disabled", domain: &Domain{ projectName: "project", - group: group{ + group: Group{ name: "group", - projects: projects{"test-domain": &project{HTTPSOnly: false}}, + projects: projects{"test-domain": &Project{HTTPSOnly: false}}, }, }, url: "http://test-domain", @@ -153,9 +153,9 @@ func TestIsHTTPSOnly(t *testing.T) { name: "Case-insensitive default group domain with HTTPS-only enabled", domain: &Domain{ projectName: "project", - group: group{ + group: Group{ name: "group", - projects: projects{"test-domain": &project{HTTPSOnly: true}}, + projects: projects{"test-domain": &Project{HTTPSOnly: true}}, }, }, url: "http://Test-domain", @@ -165,9 +165,9 @@ func TestIsHTTPSOnly(t *testing.T) { name: "Other group domain with HTTPS-only enabled", domain: &Domain{ projectName: "project", - group: group{ + group: Group{ name: "group", - projects: projects{"project": &project{HTTPSOnly: true}}, + projects: projects{"project": &Project{HTTPSOnly: true}}, }, }, url: "http://test-domain/project", @@ -177,9 +177,9 @@ func TestIsHTTPSOnly(t *testing.T) { name: "Other group domain with HTTPS-only disabled", domain: &Domain{ projectName: "project", - group: group{ + group: Group{ name: "group", - projects: projects{"project": &project{HTTPSOnly: false}}, + projects: projects{"project": &Project{HTTPSOnly: false}}, }, }, url: "http://test-domain/project", @@ -188,7 +188,7 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Unknown project", domain: &Domain{ - group: group{name: "group"}, + group: Group{name: "group"}, projectName: "project", }, url: "http://test-domain/project", @@ -217,7 +217,7 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing acme challenge", domain: &Domain{ - group: group{name: "group.acme"}, + group: Group{name: "group.acme"}, projectName: "with.acme.challenge", config: &domainConfig{HTTPSOnly: true}, }, @@ -227,7 +227,7 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing acme challenge", domain: &Domain{ - group: group{name: "group.acme"}, + group: Group{name: "group.acme"}, projectName: "with.acme.challenge", config: &domainConfig{HTTPSOnly: true}, }, @@ -237,7 +237,7 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing another token", domain: &Domain{ - group: group{name: "group.acme"}, + group: Group{name: "group.acme"}, projectName: "with.acme.challenge", config: &domainConfig{HTTPSOnly: true}, }, @@ -253,7 +253,7 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Domain without config", domain: &Domain{ - group: group{name: "group.acme"}, + group: Group{name: "group.acme"}, projectName: "with.acme.challenge", config: nil, }, @@ -301,13 +301,13 @@ func TestGroupServeHTTPGzip(t *testing.T) { testGroup := &Domain{ projectName: "", - group: group{ + group: Group{ name: "group", - projects: map[string]*project{ - "group.test.io": &project{}, - "group.gitlab-example.com": &project{}, - "project": &project{}, - "project2": &project{}, + projects: map[string]*Project{ + "group.test.io": &Project{}, + "group.gitlab-example.com": &Project{}, + "project": &Project{}, + "project2": &Project{}, }, }, } @@ -368,14 +368,14 @@ func TestGroup404ServeHTTP(t *testing.T) { testGroup := &Domain{ projectName: "", - group: group{ + group: Group{ name: "group.404", - projects: map[string]*project{ - "domain.404": &project{}, - "group.404.test.io": &project{}, - "project.404": &project{}, - "project.404.symlink": &project{}, - "project.no.404": &project{}, + projects: map[string]*Project{ + "domain.404": &Project{}, + "group.404.test.io": &Project{}, + "project.404": &Project{}, + "project.404.symlink": &Project{}, + "project.no.404": &Project{}, }, }, } @@ -396,7 +396,7 @@ func TestDomain404ServeHTTP(t *testing.T) { defer cleanup() testDomain := &Domain{ - group: group{name: "group.404"}, + group: Group{name: "group.404"}, projectName: "domain.404", config: &domainConfig{ Domain: "domain.404.com", @@ -412,7 +412,7 @@ func TestPredefined404ServeHTTP(t *testing.T) { defer cleanup() testDomain := &Domain{ - group: group{name: "group"}, + group: Group{name: "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") @@ -420,7 +420,7 @@ func TestPredefined404ServeHTTP(t *testing.T) { func TestGroupCertificate(t *testing.T) { testGroup := &Domain{ - group: group{name: "group"}, + group: Group{name: "group"}, projectName: "", } @@ -431,7 +431,7 @@ func TestGroupCertificate(t *testing.T) { func TestDomainNoCertificate(t *testing.T) { testDomain := &Domain{ - group: group{name: "group"}, + group: Group{name: "group"}, projectName: "project2", config: &domainConfig{ Domain: "test.domain.com", @@ -449,7 +449,7 @@ func TestDomainNoCertificate(t *testing.T) { func TestDomainCertificate(t *testing.T) { testDomain := &Domain{ - group: group{name: "group"}, + group: Group{name: "group"}, projectName: "project2", config: &domainConfig{ Domain: "test.domain.com", @@ -468,10 +468,10 @@ func TestCacheControlHeaders(t *testing.T) { defer cleanup() testGroup := &Domain{ - group: group{ + group: Group{ name: "group", - projects: map[string]*project{ - "group.test.io": &project{}, + projects: map[string]*Project{ + "group.test.io": &Project{}, }, }, } diff --git a/internal/domain/group.go b/internal/domain/group.go index 83b8d2556..7dcae83ec 100644 --- a/internal/domain/group.go +++ b/internal/domain/group.go @@ -5,10 +5,10 @@ import ( "strings" ) -type projects map[string]*project -type subgroups map[string]*group +type projects map[string]*Project +type subgroups map[string]*Group -type group struct { +type Group struct { name string // nested groups @@ -18,7 +18,7 @@ type group struct { projects projects } -func (g *group) digProjectWithSubpath(parentPath string, keys []string) (*project, string, string) { +func (g *Group) digProjectWithSubpath(parentPath string, keys []string) (*Project, string, string) { if len(keys) >= 1 { head := keys[0] tail := keys[1:] diff --git a/internal/domain/group_test.go b/internal/domain/group_test.go index 8f75fe96a..ac7afea10 100644 --- a/internal/domain/group_test.go +++ b/internal/domain/group_test.go @@ -8,25 +8,25 @@ import ( ) func TestGroupDig(t *testing.T) { - matchingProject := &project{ID: 1} + matchingProject := &Project{ID: 1} tests := []struct { name string - g group + g Group path string - expectedProject *project + expectedProject *Project expectedProjectPath string expectedPath string }{ { name: "empty group", path: "projectb/demo/features.html", - g: group{}, + g: Group{}, }, { name: "group with project", path: "projectb/demo/features.html", - g: group{ + g: Group{ projects: projects{"projectb": matchingProject}, }, expectedProject: matchingProject, @@ -36,7 +36,7 @@ func TestGroupDig(t *testing.T) { { name: "group with project and no path in URL", path: "projectb", - g: group{ + g: Group{ projects: projects{"projectb": matchingProject}, }, expectedProject: matchingProject, @@ -45,11 +45,11 @@ func TestGroupDig(t *testing.T) { { name: "group with subgroup and project", path: "projectb/demo/features.html", - g: group{ + g: Group{ projects: projects{"projectb": matchingProject}, subgroups: subgroups{ - "sub1": &group{ - projects: projects{"another": &project{}}, + "sub1": &Group{ + projects: projects{"another": &Project{}}, }, }, }, @@ -60,13 +60,13 @@ func TestGroupDig(t *testing.T) { { name: "group with project inside a subgroup", path: "sub1/projectb/demo/features.html", - g: group{ + g: Group{ subgroups: subgroups{ - "sub1": &group{ + "sub1": &Group{ projects: projects{"projectb": matchingProject}, }, }, - projects: projects{"another": &project{}}, + projects: projects{"another": &Project{}}, }, expectedProject: matchingProject, expectedProjectPath: "sub1/projectb", @@ -75,10 +75,10 @@ func TestGroupDig(t *testing.T) { { name: "group with matching subgroup but no project", path: "sub1/projectb/demo/features.html", - g: group{ + g: Group{ subgroups: subgroups{ - "sub1": &group{ - projects: projects{"another": &project{}}, + "sub1": &Group{ + projects: projects{"another": &Project{}}, }, }, }, diff --git a/internal/domain/map.go b/internal/domain/map.go index 7934860bb..e374bea47 100644 --- a/internal/domain/map.go +++ b/internal/domain/map.go @@ -36,7 +36,7 @@ func (dm Map) updateDomainMap(domainName string, domain *Domain) { func (dm Map) addDomain(rootDomain, groupName, projectName string, config *domainConfig) { newDomain := &Domain{ - group: group{name: groupName}, + group: Group{name: groupName}, projectName: projectName, config: config, } @@ -52,7 +52,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https if groupDomain == nil { groupDomain = &Domain{ - group: group{ + group: Group{ name: groupName, projects: make(projects), subgroups: make(subgroups), @@ -68,7 +68,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https subgroupName := split[i] subgroup := g.subgroups[subgroupName] if subgroup == nil { - subgroup = &group{ + subgroup = &Group{ name: subgroupName, projects: make(projects), subgroups: make(subgroups), @@ -79,7 +79,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https g = subgroup } - g.projects[projectName] = &project{ + g.projects[projectName] = &Project{ NamespaceProject: domainName == projectName, HTTPSOnly: httpsOnly, AccessControl: accessControl, diff --git a/internal/domain/map_test.go b/internal/domain/map_test.go index de5e49552..2d61d248f 100644 --- a/internal/domain/map_test.go +++ b/internal/domain/map_test.go @@ -77,7 +77,7 @@ func TestReadProjects(t *testing.T) { // check subgroups domain, ok := dm["group.test.io"] require.True(t, ok, "missing group.test.io domain") - subgroup, ok := domain.subgroups["subgroup"] + subgroup, ok := domain.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") diff --git a/internal/domain/project.go b/internal/domain/project.go new file mode 100644 index 000000000..2d38ff746 --- /dev/null +++ b/internal/domain/project.go @@ -0,0 +1,8 @@ +package domain + +type Project struct { + NamespaceProject bool + HTTPSOnly bool + AccessControl bool + ID uint64 +} -- GitLab From 529988bfe243a610ea5414a48567585c3c3337aa Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 20 Sep 2019 14:18:12 +0200 Subject: [PATCH 02/13] Make it more explicit how domain config is structured --- go.mod | 2 -- .../domain/{domain_config.go => config.go} | 13 ++++++----- .../{domain_config_test.go => config_test.go} | 22 +++++++++---------- internal/domain/domain.go | 2 +- internal/domain/domain_test.go | 18 +++++++-------- internal/domain/group.go | 7 +++--- internal/domain/map.go | 10 ++++----- internal/domain/map_test.go | 4 ++-- internal/domain/project.go | 1 + 9 files changed, 41 insertions(+), 38 deletions(-) rename internal/domain/{domain_config.go => config.go} (69%) rename internal/domain/{domain_config_test.go => config_test.go} (77%) 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_config.go b/internal/domain/config.go similarity index 69% rename from internal/domain/domain_config.go rename to internal/domain/config.go index 2ab2ce6cb..0836eedd9 100644 --- a/internal/domain/domain_config.go +++ b/internal/domain/config.go @@ -7,7 +7,8 @@ import ( "strings" ) -type domainConfig struct { +// Config represents a custom domain config +type Config struct { Domain string Certificate string Key string @@ -16,14 +17,16 @@ type domainConfig struct { AccessControl bool `json:"access_control"` } -type domainsConfig struct { - Domains []domainConfig +// MultiConfig represents a group of custom domain configs +type MultiConfig struct { + Domains []Config HTTPSOnly bool `json:"https_only"` ID uint64 `json:"id"` AccessControl bool `json:"access_control"` } -func (c *domainConfig) Valid(rootDomain string) bool { +// Valid validates a custom domain config for a root domain +func (c *Config) Valid(rootDomain string) bool { if c.Domain == "" { return false } @@ -34,7 +37,7 @@ func (c *domainConfig) Valid(rootDomain string) bool { return !strings.HasSuffix(domain, rootDomain) } -func (c *domainsConfig) Read(group, project string) (err error) { +func (c *MultiConfig) Read(group, project string) (err error) { configFile, err := os.Open(filepath.Join(group, project, "config.json")) if err != nil { return err diff --git a/internal/domain/domain_config_test.go b/internal/domain/config_test.go similarity index 77% rename from internal/domain/domain_config_test.go rename to internal/domain/config_test.go index 8cdcdeaa6..a78c72cf1 100644 --- a/internal/domain/domain_config_test.go +++ b/internal/domain/config_test.go @@ -14,25 +14,25 @@ const invalidConfig = `{"Domains":{}}` const validConfig = `{"Domains":[{"Domain":"test"}]}` func TestDomainConfigValidness(t *testing.T) { - d := domainConfig{} + d := Config{} require.False(t, d.Valid("gitlab.io")) - d = domainConfig{Domain: "test"} + d = Config{Domain: "test"} require.True(t, d.Valid("gitlab.io")) - d = domainConfig{Domain: "test"} + d = Config{Domain: "test"} require.True(t, d.Valid("gitlab.io")) - d = domainConfig{Domain: "test.gitlab.io"} + d = Config{Domain: "test.gitlab.io"} require.False(t, d.Valid("gitlab.io")) - d = domainConfig{Domain: "test.test.gitlab.io"} + d = Config{Domain: "test.test.gitlab.io"} require.False(t, d.Valid("gitlab.io")) - d = domainConfig{Domain: "test.testgitlab.io"} + d = Config{Domain: "test.testgitlab.io"} require.True(t, d.Valid("gitlab.io")) - d = domainConfig{Domain: "test.GitLab.Io"} + d = Config{Domain: "test.GitLab.Io"} require.False(t, d.Valid("gitlab.io")) } @@ -40,26 +40,26 @@ func TestDomainConfigRead(t *testing.T) { cleanup := setUpTests(t) defer cleanup() - d := domainsConfig{} + d := MultiConfig{} err := d.Read("test-group", "test-project") require.Error(t, err) os.MkdirAll(filepath.Dir(configFile), 0700) defer os.RemoveAll("test-group") - d = domainsConfig{} + d = MultiConfig{} err = d.Read("test-group", "test-project") require.Error(t, err) err = ioutil.WriteFile(configFile, []byte(invalidConfig), 0600) require.NoError(t, err) - d = domainsConfig{} + d = MultiConfig{} err = d.Read("test-group", "test-project") require.Error(t, err) err = ioutil.WriteFile(configFile, []byte(validConfig), 0600) require.NoError(t, err) - d = domainsConfig{} + d = MultiConfig{} err = d.Read("test-group", "test-project") require.NoError(t, err) } diff --git a/internal/domain/domain.go b/internal/domain/domain.go index efea118a3..0c464628e 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -44,7 +44,7 @@ type Domain struct { // custom domains: projectName string - config *domainConfig + config *Config certificate *tls.Certificate certificateError error diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 1f200afae..41a0c401a 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -82,7 +82,7 @@ func TestDomainServeHTTP(t *testing.T) { testDomain := &Domain{ group: Group{name: "group"}, projectName: "project2", - config: &domainConfig{ + config: &Config{ Domain: "test.domain.com", }, } @@ -110,7 +110,7 @@ func TestIsHTTPSOnly(t *testing.T) { domain: &Domain{ group: Group{name: "group"}, projectName: "project", - config: &domainConfig{HTTPSOnly: true}, + config: &Config{HTTPSOnly: true}, }, url: "http://custom-domain", expected: true, @@ -120,7 +120,7 @@ func TestIsHTTPSOnly(t *testing.T) { domain: &Domain{ group: Group{name: "group"}, projectName: "project", - config: &domainConfig{HTTPSOnly: false}, + config: &Config{HTTPSOnly: false}, }, url: "http://custom-domain", expected: false, @@ -219,7 +219,7 @@ func TestHasAcmeChallenge(t *testing.T) { domain: &Domain{ group: Group{name: "group.acme"}, projectName: "with.acme.challenge", - config: &domainConfig{HTTPSOnly: true}, + config: &Config{HTTPSOnly: true}, }, token: "existingtoken", expected: true, @@ -229,7 +229,7 @@ func TestHasAcmeChallenge(t *testing.T) { domain: &Domain{ group: Group{name: "group.acme"}, projectName: "with.acme.challenge", - config: &domainConfig{HTTPSOnly: true}, + config: &Config{HTTPSOnly: true}, }, token: "foldertoken", expected: true, @@ -239,7 +239,7 @@ func TestHasAcmeChallenge(t *testing.T) { domain: &Domain{ group: Group{name: "group.acme"}, projectName: "with.acme.challenge", - config: &domainConfig{HTTPSOnly: true}, + config: &Config{HTTPSOnly: true}, }, token: "notexistingtoken", expected: false, @@ -398,7 +398,7 @@ func TestDomain404ServeHTTP(t *testing.T) { testDomain := &Domain{ group: Group{name: "group.404"}, projectName: "domain.404", - config: &domainConfig{ + config: &Config{ Domain: "domain.404.com", }, } @@ -433,7 +433,7 @@ func TestDomainNoCertificate(t *testing.T) { testDomain := &Domain{ group: Group{name: "group"}, projectName: "project2", - config: &domainConfig{ + config: &Config{ Domain: "test.domain.com", }, } @@ -451,7 +451,7 @@ func TestDomainCertificate(t *testing.T) { testDomain := &Domain{ group: Group{name: "group"}, projectName: "project2", - config: &domainConfig{ + config: &Config{ Domain: "test.domain.com", Certificate: fixture.Certificate, Key: fixture.Key, diff --git a/internal/domain/group.go b/internal/domain/group.go index 7dcae83ec..e3424a4d0 100644 --- a/internal/domain/group.go +++ b/internal/domain/group.go @@ -5,9 +5,7 @@ import ( "strings" ) -type projects map[string]*Project -type subgroups map[string]*Group - +// Group represents a GitLab group with projects and subgroups type Group struct { name string @@ -18,6 +16,9 @@ type Group struct { projects projects } +type projects map[string]*Project +type subgroups map[string]*Group + func (g *Group) digProjectWithSubpath(parentPath string, keys []string) (*Project, string, string) { if len(keys) >= 1 { head := keys[0] diff --git a/internal/domain/map.go b/internal/domain/map.go index e374bea47..f65f52af4 100644 --- a/internal/domain/map.go +++ b/internal/domain/map.go @@ -15,7 +15,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/metrics" ) -// Map maps domain names to D instances. +// Map maps domain names to Domain instances. type Map map[string]*Domain type domainsUpdater func(Map) @@ -34,7 +34,7 @@ func (dm Map) updateDomainMap(domainName string, domain *Domain) { dm[domainName] = domain } -func (dm Map) addDomain(rootDomain, groupName, projectName string, config *domainConfig) { +func (dm Map) addDomain(rootDomain, groupName, projectName string, config *Config) { newDomain := &Domain{ group: Group{name: groupName}, projectName: projectName, @@ -89,7 +89,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https dm[domainName] = groupDomain } -func (dm Map) readProjectConfig(rootDomain string, group, projectName string, config *domainsConfig) { +func (dm Map) readProjectConfig(rootDomain string, group, projectName string, config *MultiConfig) { if config == nil { // This is necessary to preserve the previous behaviour where a // group domain is created even if no config.json files are @@ -131,7 +131,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 := &domainsConfig{} + config := &MultiConfig{} if err := config.Read(group, projectPath); err != nil { config = nil } @@ -163,7 +163,7 @@ func readProjects(group, parent string, level int, buf []byte, fanIn chan<- jobR type jobResult struct { group string project string - config *domainsConfig + config *MultiConfig } // ReadGroups walks the pages directory and populates dm with all the domains it finds. diff --git a/internal/domain/map_test.go b/internal/domain/map_test.go index 2d61d248f..f184672ce 100644 --- a/internal/domain/map_test.go +++ b/internal/domain/map_test.go @@ -68,10 +68,10 @@ func TestReadProjects(t *testing.T) { } // Check that multiple domains in the same project are recorded faithfully - exp1 := &domainConfig{Domain: "test.domain.com"} + exp1 := &Config{Domain: "test.domain.com"} require.Equal(t, exp1, dm["test.domain.com"].config) - exp2 := &domainConfig{Domain: "other.domain.com", Certificate: "test", Key: "key"} + exp2 := &Config{Domain: "other.domain.com", Certificate: "test", Key: "key"} require.Equal(t, exp2, dm["other.domain.com"].config) // check subgroups diff --git a/internal/domain/project.go b/internal/domain/project.go index 2d38ff746..d0add00d5 100644 --- a/internal/domain/project.go +++ b/internal/domain/project.go @@ -1,5 +1,6 @@ package domain +// Project represents GitLab project settings type Project struct { NamespaceProject bool HTTPSOnly bool -- GitLab From e33a670211b5a63b5db4c024b95ec2d96d2ef463 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 21 Sep 2019 14:03:57 +0200 Subject: [PATCH 03/13] Add abstract interfaces for a domain source nad serving We use Source to get an information about a domain, and we use Serving to handle a request. --- internal/serving/handler.go | 7 +++++++ internal/serving/serving.go | 9 +++++++++ internal/source/source.go | 14 ++++++++++++++ 3 files changed, 30 insertions(+) create mode 100644 internal/serving/handler.go create mode 100644 internal/serving/serving.go create mode 100644 internal/source/source.go diff --git a/internal/serving/handler.go b/internal/serving/handler.go new file mode 100644 index 000000000..60b8dba90 --- /dev/null +++ b/internal/serving/handler.go @@ -0,0 +1,7 @@ +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 new file mode 100644 index 000000000..bc0bb61db --- /dev/null +++ b/internal/serving/serving.go @@ -0,0 +1,9 @@ +package serving + +import "net/http" + +// Serving represents an interface used to serve pages for a given domain / +// address +type Serving interface { + ServeHTTP(http.ResponseWriter, *http.Request) +} diff --git a/internal/source/source.go b/internal/source/source.go new file mode 100644 index 000000000..23dc72fe1 --- /dev/null +++ b/internal/source/source.go @@ -0,0 +1,14 @@ +package source + +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" +) + +// Source represents a source of information about a domain. Whenever a request +// appears a concret implementation of a Source should find a domain that is +// needed to handle the request and serve pages +type Source interface { + GetDomain(*http.Request) *domain.Domain +} -- GitLab From 079f5e979142bceeeb6506c4d31d7c9f1488ce78 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 21 Sep 2019 16:17:03 +0200 Subject: [PATCH 04/13] Separate domain config source from a domain --- app.go | 7 +- internal/auth/auth.go | 8 +- internal/auth/auth_test.go | 10 +- internal/domain/domain.go | 154 +++--- internal/domain/domain_test.go | 334 ++----------- internal/domain/group.go | 39 -- internal/domain/project.go | 9 - internal/{domain => source/dirs}/config.go | 28 +- .../{domain => source/dirs}/config_test.go | 24 +- internal/source/dirs/group.go | 135 ++++++ internal/source/dirs/group_domain_test.go | 440 ++++++++++++++++++ .../{domain => source/dirs}/group_test.go | 12 +- internal/{domain => source/dirs}/map.go | 45 +- internal/{domain => source/dirs}/map_test.go | 15 +- 14 files changed, 780 insertions(+), 480 deletions(-) delete mode 100644 internal/domain/group.go delete mode 100644 internal/domain/project.go rename internal/{domain => source/dirs}/config.go (54%) rename internal/{domain => source/dirs}/config_test.go (75%) create mode 100644 internal/source/dirs/group.go create mode 100644 internal/source/dirs/group_domain_test.go rename internal/{domain => source/dirs}/group_test.go (89%) rename internal/{domain => source/dirs}/map.go (87%) rename internal/{domain => source/dirs}/map_test.go (94%) diff --git a/app.go b/app.go index 9be4409c2..c14d7bc2a 100644 --- a/app.go +++ b/app.go @@ -28,6 +28,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/logging" "gitlab.com/gitlab-org/gitlab-pages/internal/netutil" "gitlab.com/gitlab-org/gitlab-pages/internal/request" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/dirs" ) const ( @@ -47,7 +48,7 @@ var ( type theApp struct { appConfig - dm domain.Map + dm dirs.Map lock sync.RWMutex Artifact *artifact.Artifact Auth *auth.Auth @@ -322,7 +323,7 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { return handler, nil } -func (a *theApp) UpdateDomains(dm domain.Map) { +func (a *theApp) UpdateDomains(dm dirs.Map) { a.lock.Lock() defer a.lock.Unlock() a.dm = dm @@ -366,7 +367,7 @@ func (a *theApp) Run() { a.listenAdminUnix(&wg) a.listenAdminHTTPS(&wg) - go domain.Watch(a.Domain, a.UpdateDomains, time.Second) + go dirs.Watch(a.Domain, a.UpdateDomains, time.Second) wg.Wait() } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 77bc7d8e9..154d86dae 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -19,10 +19,10 @@ import ( log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/labkit/errortracking" - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" "gitlab.com/gitlab-org/gitlab-pages/internal/request" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/dirs" "golang.org/x/crypto/hkdf" ) @@ -108,7 +108,7 @@ func (a *Auth) checkSession(w http.ResponseWriter, r *http.Request) (*sessions.S } // TryAuthenticate tries to authenticate user and fetch access token if request is a callback to auth -func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, dm domain.Map, lock *sync.RWMutex) bool { +func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, dm dirs.Map, lock *sync.RWMutex) bool { if a == nil { return false @@ -200,7 +200,7 @@ func (a *Auth) checkAuthenticationResponse(session *sessions.Session, w http.Res http.Redirect(w, r, redirectURI, 302) } -func (a *Auth) domainAllowed(domain string, dm domain.Map, lock *sync.RWMutex) bool { +func (a *Auth) domainAllowed(domain string, dm dirs.Map, lock *sync.RWMutex) bool { lock.RLock() defer lock.RUnlock() @@ -209,7 +209,7 @@ func (a *Auth) domainAllowed(domain string, dm domain.Map, lock *sync.RWMutex) b return domain == a.pagesDomain || strings.HasSuffix("."+domain, a.pagesDomain) || present } -func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, dm domain.Map, lock *sync.RWMutex) bool { +func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, dm dirs.Map, lock *sync.RWMutex) bool { // If request is for authenticating via custom domain if shouldProxyAuth(r) { domain := r.URL.Query().Get("domain") diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 8be5e8354..8102a5d18 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -11,8 +11,8 @@ import ( "github.com/gorilla/sessions" "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/request" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/dirs" ) func createAuth(t *testing.T) *Auth { @@ -55,7 +55,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, make(domain.Map), &sync.RWMutex{})) + require.Equal(t, false, auth.TryAuthenticate(result, r, make(dirs.Map), &sync.RWMutex{})) } func TestTryAuthenticateWithError(t *testing.T) { @@ -66,7 +66,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, make(domain.Map), &sync.RWMutex{})) + require.Equal(t, true, auth.TryAuthenticate(result, r, make(dirs.Map), &sync.RWMutex{})) require.Equal(t, 401, result.Code) } @@ -83,7 +83,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) { session.Values["state"] = "state" session.Save(r, result) - require.Equal(t, true, auth.TryAuthenticate(result, r, make(domain.Map), &sync.RWMutex{})) + require.Equal(t, true, auth.TryAuthenticate(result, r, make(dirs.Map), &sync.RWMutex{})) require.Equal(t, 401, result.Code) } @@ -123,7 +123,7 @@ func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) { }) result := httptest.NewRecorder() - require.Equal(t, true, auth.TryAuthenticate(result, r, make(domain.Map), &sync.RWMutex{})) + require.Equal(t, true, auth.TryAuthenticate(result, r, make(dirs.Map), &sync.RWMutex{})) 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/domain/domain.go b/internal/domain/domain.go index 0c464628e..1c90a2322 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -16,19 +16,10 @@ import ( "golang.org/x/sys/unix" - "gitlab.com/gitlab-org/gitlab-pages/internal/host" "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/httputil" ) -const ( - subgroupScanLimit int = 21 - // maxProjectDepth is set to the maximum nested project depth in gitlab (21) plus 3. - // One for the project, one for the first empty element of the split (URL.Path starts with /), - // and one for the real file path - maxProjectDepth int = subgroupScanLimit + 3 -) - type locationDirectoryError struct { FullPath string RelativePath string @@ -38,13 +29,28 @@ type locationFileNoExtensionError struct { FullPath string } +type GroupConfig interface { + IsHTTPSOnly(*http.Request) (bool, error) + HasAccessControl(*http.Request) (bool, error) + IsNamespaceProject(*http.Request) (bool, error) + ProjectID(*http.Request) (uint64, error) + ProjectExists(*http.Request) (bool, error) + ProjectWithSubpath(*http.Request) (string, string, error) +} + // Domain is a domain that gitlab-pages can serve. type Domain struct { - group Group + Group string + Project string + + DomainName string + Certificate string + Key string + HTTPSOnly bool + ProjectID uint64 + AccessControl bool - // custom domains: - projectName string - config *Config + GroupConfig GroupConfig // handles group domain config certificate *tls.Certificate certificateError error @@ -53,15 +59,19 @@ type Domain struct { // String implements Stringer. func (d *Domain) String() string { - if d.group.name != "" && d.projectName != "" { - return d.group.name + "/" + d.projectName + if d.Group != "" && d.Project != "" { + return d.Group + "/" + d.Project } - if d.group.name != "" { - return d.group.name + if d.Group != "" { + return d.Group } - return d.projectName + return d.Project +} + +func (d *Domain) isCustomDomain() bool { + return d.GroupConfig == nil } func (l *locationDirectoryError) Error() string { @@ -99,30 +109,6 @@ func handleGZip(w http.ResponseWriter, r *http.Request, fullPath string) string return gzipPath } -// Look up a project inside the domain based on the host and path. Returns the -// project and its name (if applicable) -func (d *Domain) getProjectWithSubpath(r *http.Request) (*Project, 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 { - project, projectPath, urlPath := d.group.digProjectWithSubpath("", split[1:]) - if project != nil { - return project, projectPath, urlPath - } - } - - // Since the URL doesn't specify a project (e.g. http://mydomain.gitlab.io), - // return the group project if it exists. - if host := host.FromRequest(r); host != "" { - if groupProject := d.group.projects[host]; groupProject != nil { - return groupProject, host, strings.Join(split[1:], "/") - } - } - - return 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 { @@ -131,13 +117,18 @@ func (d *Domain) IsHTTPSOnly(r *http.Request) bool { } // Check custom domain config (e.g. http://example.com) - if d.config != nil { - return d.config.HTTPSOnly + // if d.!= nil { + if d.isCustomDomain() { + return d.HTTPSOnly } // Check projects served under the group domain, including the default one - if project, _, _ := d.getProjectWithSubpath(r); project != nil { - return project.HTTPSOnly + // TODO REFACTORING + // if project, _, _ := d.getProjectWithSubpath(r); project != nil { + // return project.HTTPSOnly + // } + if httpsOnly, err := d.GroupConfig.IsHTTPSOnly(r); err == nil { + return httpsOnly } return false @@ -150,13 +141,16 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { } // Check custom domain config (e.g. http://example.com) - if d.config != nil { - return d.config.AccessControl + if d.isCustomDomain() { + return d.AccessControl } // Check projects served under the group domain, including the default one - if project, _, _ := d.getProjectWithSubpath(r); project != nil { - return project.AccessControl + // TODO RFR if project, _, _ := d.getProjectWithSubpath(r); project != nil { + // return project.AccessControl + //} + if hasAccessControl, err := d.GroupConfig.HasAccessControl(r); err == nil { + return hasAccessControl } return false @@ -168,18 +162,18 @@ func (d *Domain) HasAcmeChallenge(token string) bool { return false } - if d.config == nil { + if !d.isCustomDomain() { return false } - _, err := d.resolvePath(d.projectName, ".well-known/acme-challenge", token) + _, 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.projectName, ".well-known/acme-challenge", token, "index.html") + _, err = d.resolvePath(d.Project, ".well-known/acme-challenge", token, "index.html") if err == nil { return true @@ -196,13 +190,16 @@ 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.config != nil { + if d.isCustomDomain() { return false } // Check projects served under the group domain, including the default one - if project, _, _ := d.getProjectWithSubpath(r); project != nil { - return project.NamespaceProject + // if project, _, _ := d.getProjectWithSubpath(r); project != nil { + // return project.NamespaceProject + // } + if isNamespaceProject, err := d.GroupConfig.IsNamespaceProject(r); err == nil { + return isNamespaceProject } return false @@ -214,12 +211,15 @@ func (d *Domain) GetID(r *http.Request) uint64 { return 0 } - if d.config != nil { - return d.config.ID + if d.isCustomDomain() { + return d.ProjectID } - if project, _, _ := d.getProjectWithSubpath(r); project != nil { - return project.ID + // if project, _, _ := d.getProjectWithSubpath(r); project != nil { + // return project.ID + // } + if projectID, err := d.GroupConfig.ProjectID(r); err == nil { + return projectID } return 0 @@ -231,12 +231,15 @@ func (d *Domain) HasProject(r *http.Request) bool { return false } - if d.config != nil { + if d.isCustomDomain() { return true } - if project, _, _ := d.getProjectWithSubpath(r); project != nil { - return true + // if project, _, _ := d.getProjectWithSubpath(r); project != nil { + // return true + // } + if projectExists, err := d.GroupConfig.ProjectExists(r); err == nil { + return projectExists } return false @@ -334,7 +337,7 @@ func (d *Domain) serveCustomFile(w http.ResponseWriter, r *http.Request, code in // 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.name, projectName, "public") + 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 @@ -421,8 +424,9 @@ func (d *Domain) tryFile(w http.ResponseWriter, r *http.Request, projectName str } func (d *Domain) serveFileFromGroup(w http.ResponseWriter, r *http.Request) bool { - project, projectName, subPath := d.getProjectWithSubpath(r) - if project == nil { + // project, projectName, subPath := d.getProjectWithSubpath(r) + projectName, subPath, err := d.GroupConfig.ProjectWithSubpath(r) + if err != nil { httperrors.Serve404(w) return true } @@ -435,8 +439,10 @@ func (d *Domain) serveFileFromGroup(w http.ResponseWriter, r *http.Request) bool } func (d *Domain) serveNotFoundFromGroup(w http.ResponseWriter, r *http.Request) { - project, projectName, _ := d.getProjectWithSubpath(r) - if project == nil { + // project, projectName, _ := d.getProjectWithSubpath(r) + projectName, _, err := d.GroupConfig.ProjectWithSubpath(r) + + if err != nil { httperrors.Serve404(w) return } @@ -452,7 +458,7 @@ func (d *Domain) serveNotFoundFromGroup(w http.ResponseWriter, r *http.Request) 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.projectName, r.URL.Path) == nil { + if d.tryFile(w, r, d.Project, r.URL.Path) == nil { return true } @@ -461,7 +467,7 @@ func (d *Domain) serveFileFromConfig(w http.ResponseWriter, r *http.Request) boo 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.projectName) == nil { + if d.tryNotFound(w, r, d.Project) == nil { return } @@ -471,13 +477,13 @@ func (d *Domain) serveNotFoundFromConfig(w http.ResponseWriter, r *http.Request) // EnsureCertificate parses the PEM-encoded certificate for the domain func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { - if d.config == nil { + if !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.config.Certificate), []byte(d.config.Key)) + cert, d.certificateError = tls.X509KeyPair([]byte(d.Certificate), []byte(d.Key)) if d.certificateError == nil { d.certificate = &cert } @@ -493,7 +499,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { return true } - if d.config != nil { + if d.isCustomDomain() { return d.serveFileFromConfig(w, r) } @@ -507,7 +513,7 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { return } - if d.config != nil { + if d.isCustomDomain() { d.serveNotFoundFromConfig(w, r) } else { d.serveNotFoundFromGroup(w, r) diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 41a0c401a..552b2f15d 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -8,7 +8,6 @@ import ( "net/url" "os" "testing" - "time" "github.com/stretchr/testify/require" @@ -24,67 +23,14 @@ func serveFileOrNotFound(domain *Domain) http.HandlerFunc { } } -func testGroupServeHTTPHost(t *testing.T, host string) { - testGroup := &Domain{ - projectName: "", - group: Group{ - name: "group", - projects: map[string]*Project{ - "group.test.io": &Project{}, - "group.gitlab-example.com": &Project{}, - "project": &Project{}, - "project2": &Project{}, - }, - }, - } - - makeURL := func(path string) string { - return "http://" + host + path - } - - serve := serveFileOrNotFound(testGroup) - - require.HTTPBodyContains(t, serve, "GET", makeURL("/"), nil, "main-dir") - require.HTTPBodyContains(t, serve, "GET", makeURL("/index"), nil, "main-dir") - require.HTTPBodyContains(t, serve, "GET", makeURL("/index.html"), nil, "main-dir") - testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project"), nil, "//"+host+"/project/") - require.HTTPBodyContains(t, serve, "GET", makeURL("/project/"), nil, "project-subdir") - require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index"), nil, "project-subdir") - require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index/"), nil, "project-subdir") - require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index.html"), nil, "project-subdir") - testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project/subdir"), nil, "//"+host+"/project/subdir/") - require.HTTPBodyContains(t, serve, "GET", makeURL("/project/subdir/"), nil, "project-subsubdir") - require.HTTPBodyContains(t, serve, "GET", makeURL("/project2/"), nil, "project2-main") - require.HTTPBodyContains(t, serve, "GET", makeURL("/project2/index"), nil, "project2-main") - require.HTTPBodyContains(t, serve, "GET", makeURL("/project2/index.html"), nil, "project2-main") - require.HTTPError(t, serve, "GET", makeURL("/private.project/"), nil) - require.HTTPError(t, serve, "GET", makeURL("//about.gitlab.com/%2e%2e"), nil) - require.HTTPError(t, serve, "GET", makeURL("/symlink"), nil) - require.HTTPError(t, serve, "GET", makeURL("/symlink/index.html"), nil) - require.HTTPError(t, serve, "GET", makeURL("/symlink/subdir/"), nil) - require.HTTPError(t, serve, "GET", makeURL("/project/fifo"), nil) - require.HTTPError(t, serve, "GET", makeURL("/not-existing-file"), nil) - require.HTTPRedirect(t, serve, "GET", makeURL("/project//about.gitlab.com/%2e%2e"), nil) -} - -func TestGroupServeHTTP(t *testing.T) { - cleanup := setUpTests(t) - defer cleanup() - - t.Run("group.test.io", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io") }) - t.Run("group.test.io:8080", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io:8080") }) -} - func TestDomainServeHTTP(t *testing.T) { cleanup := setUpTests(t) defer cleanup() testDomain := &Domain{ - group: Group{name: "group"}, - projectName: "project2", - config: &Config{ - Domain: "test.domain.com", - }, + Project: "project2", + Group: "group", + DomainName: "test.domain.com", } require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/", nil, "project2-main") @@ -108,9 +54,9 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only enabled", domain: &Domain{ - group: Group{name: "group"}, - projectName: "project", - config: &Config{HTTPSOnly: true}, + Project: "project", + Group: "group", + HTTPSOnly: true, }, url: "http://custom-domain", expected: true, @@ -118,78 +64,18 @@ func TestIsHTTPSOnly(t *testing.T) { { name: "Custom domain with HTTPS-only disabled", domain: &Domain{ - group: Group{name: "group"}, - projectName: "project", - config: &Config{HTTPSOnly: false}, + Project: "project", + Group: "group", + HTTPSOnly: false, }, url: "http://custom-domain", expected: false, }, - { - name: "Default group domain with HTTPS-only enabled", - domain: &Domain{ - projectName: "project", - group: Group{ - name: "group", - projects: projects{"test-domain": &Project{HTTPSOnly: true}}, - }, - }, - url: "http://test-domain", - expected: true, - }, - { - name: "Default group domain with HTTPS-only disabled", - domain: &Domain{ - projectName: "project", - group: Group{ - name: "group", - projects: projects{"test-domain": &Project{HTTPSOnly: false}}, - }, - }, - url: "http://test-domain", - expected: false, - }, - { - name: "Case-insensitive default group domain with HTTPS-only enabled", - domain: &Domain{ - projectName: "project", - group: Group{ - name: "group", - projects: projects{"test-domain": &Project{HTTPSOnly: true}}, - }, - }, - url: "http://Test-domain", - expected: true, - }, - { - name: "Other group domain with HTTPS-only enabled", - domain: &Domain{ - projectName: "project", - group: Group{ - name: "group", - projects: projects{"project": &Project{HTTPSOnly: true}}, - }, - }, - url: "http://test-domain/project", - expected: true, - }, - { - name: "Other group domain with HTTPS-only disabled", - domain: &Domain{ - projectName: "project", - group: Group{ - name: "group", - projects: projects{"project": &Project{HTTPSOnly: false}}, - }, - }, - url: "http://test-domain/project", - expected: false, - }, { name: "Unknown project", domain: &Domain{ - group: Group{name: "group"}, - projectName: "project", + Project: "project", + Group: "group", }, url: "http://test-domain/project", expected: false, @@ -217,9 +103,9 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing acme challenge", domain: &Domain{ - group: Group{name: "group.acme"}, - projectName: "with.acme.challenge", - config: &Config{HTTPSOnly: true}, + Group: "group.acme", + Project: "with.acme.challenge", + HTTPSOnly: true, }, token: "existingtoken", expected: true, @@ -227,9 +113,9 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing acme challenge", domain: &Domain{ - group: Group{name: "group.acme"}, - projectName: "with.acme.challenge", - config: &Config{HTTPSOnly: true}, + Group: "group.acme", + Project: "with.acme.challenge", + HTTPSOnly: true, }, token: "foldertoken", expected: true, @@ -237,9 +123,9 @@ func TestHasAcmeChallenge(t *testing.T) { { name: "Project containing another token", domain: &Domain{ - group: Group{name: "group.acme"}, - projectName: "with.acme.challenge", - config: &Config{HTTPSOnly: true}, + Group: "group.acme", + Project: "with.acme.challenge", + HTTPSOnly: true, }, token: "notexistingtoken", expected: false, @@ -250,16 +136,15 @@ func TestHasAcmeChallenge(t *testing.T) { token: "existingtoken", expected: false, }, - { - name: "Domain without config", - domain: &Domain{ - group: Group{name: "group.acme"}, - projectName: "with.acme.challenge", - config: nil, - }, - 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, + // }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -295,112 +180,14 @@ func testHTTPGzip(t *testing.T, handler http.HandlerFunc, mode, url string, valu require.Equal(t, contentType, w.Header().Get("Content-Type")) } -func TestGroupServeHTTPGzip(t *testing.T) { - cleanup := setUpTests(t) - defer cleanup() - - testGroup := &Domain{ - projectName: "", - group: Group{ - name: "group", - projects: map[string]*Project{ - "group.test.io": &Project{}, - "group.gitlab-example.com": &Project{}, - "project": &Project{}, - "project2": &Project{}, - }, - }, - } - - testSet := []struct { - mode string // HTTP mode - url string // Test URL - acceptEncoding string // Accept encoding header - body interface{} // Expected body at above URL - contentType string // Expected content-type - ungzip bool // Expect the response to be gzipped? - }{ - // No gzip encoding requested - {"GET", "/index.html", "", "main-dir", "text/html; charset=utf-8", false}, - {"GET", "/index.html", "identity", "main-dir", "text/html; charset=utf-8", false}, - {"GET", "/index.html", "gzip; q=0", "main-dir", "text/html; charset=utf-8", false}, - // gzip encoding requested, - {"GET", "/index.html", "*", "main-dir", "text/html; charset=utf-8", true}, - {"GET", "/index.html", "identity, gzip", "main-dir", "text/html; charset=utf-8", true}, - {"GET", "/index.html", "gzip", "main-dir", "text/html; charset=utf-8", true}, - {"GET", "/index.html", "gzip; q=1", "main-dir", "text/html; charset=utf-8", true}, - {"GET", "/index.html", "gzip; q=0.9", "main-dir", "text/html; charset=utf-8", true}, - {"GET", "/index.html", "gzip, deflate", "main-dir", "text/html; charset=utf-8", true}, - {"GET", "/index.html", "gzip; q=1, deflate", "main-dir", "text/html; charset=utf-8", true}, - {"GET", "/index.html", "gzip; q=0.9, deflate", "main-dir", "text/html; charset=utf-8", true}, - // gzip encoding requested, but url does not have compressed content on disk - {"GET", "/project2/index.html", "*", "project2-main", "text/html; charset=utf-8", false}, - {"GET", "/project2/index.html", "identity, gzip", "project2-main", "text/html; charset=utf-8", false}, - {"GET", "/project2/index.html", "gzip", "project2-main", "text/html; charset=utf-8", false}, - {"GET", "/project2/index.html", "gzip; q=1", "project2-main", "text/html; charset=utf-8", false}, - {"GET", "/project2/index.html", "gzip; q=0.9", "project2-main", "text/html; charset=utf-8", false}, - {"GET", "/project2/index.html", "gzip, deflate", "project2-main", "text/html; charset=utf-8", false}, - {"GET", "/project2/index.html", "gzip; q=1, deflate", "project2-main", "text/html; charset=utf-8", false}, - {"GET", "/project2/index.html", "gzip; q=0.9, deflate", "project2-main", "text/html; charset=utf-8", false}, - // malformed headers - {"GET", "/index.html", ";; gzip", "main-dir", "text/html; charset=utf-8", false}, - {"GET", "/index.html", "middle-out", "main-dir", "text/html; charset=utf-8", false}, - {"GET", "/index.html", "gzip; quality=1", "main-dir", "text/html; charset=utf-8", false}, - // Symlinked .gz files are not supported - {"GET", "/gz-symlink", "*", "data", "text/plain; charset=utf-8", false}, - // Unknown file-extension, with text content - {"GET", "/text.unknown", "*", "hello", "text/plain; charset=utf-8", true}, - {"GET", "/text-nogzip.unknown", "*", "hello", "text/plain; charset=utf-8", false}, - // Unknown file-extension, with PNG content - {"GET", "/image.unknown", "*", "GIF89a", "image/gif", true}, - {"GET", "/image-nogzip.unknown", "*", "GIF89a", "image/gif", false}, - } - - for _, tt := range testSet { - URL := "http://group.test.io" + tt.url - testHTTPGzip(t, serveFileOrNotFound(testGroup), tt.mode, URL, nil, tt.acceptEncoding, tt.body, tt.contentType, tt.ungzip) - } -} - -func TestGroup404ServeHTTP(t *testing.T) { - cleanup := setUpTests(t) - defer cleanup() - - testGroup := &Domain{ - projectName: "", - group: Group{ - name: "group.404", - projects: map[string]*Project{ - "domain.404": &Project{}, - "group.404.test.io": &Project{}, - "project.404": &Project{}, - "project.404.symlink": &Project{}, - "project.no.404": &Project{}, - }, - }, - } - - testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/not/existing-file", nil, "Custom 404 project page") - testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/", nil, "Custom 404 project page") - testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not/existing-file", nil, "Custom 404 group page") - testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") - testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") - require.HTTPBodyNotContains(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404.symlink/not/existing-file", nil, "Custom 404 project page") - - // Ensure the namespace project's custom 404.html is not used by projects - testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.no.404/not/existing-file", nil, "The page you're looking for could not be found.") -} - func TestDomain404ServeHTTP(t *testing.T) { cleanup := setUpTests(t) defer cleanup() testDomain := &Domain{ - group: Group{name: "group.404"}, - projectName: "domain.404", - config: &Config{ - Domain: "domain.404.com", - }, + Group: "group.404", + Project: "domain.404", + DomainName: "domain.404.com", } testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") @@ -412,7 +199,7 @@ func TestPredefined404ServeHTTP(t *testing.T) { defer cleanup() testDomain := &Domain{ - group: Group{name: "group"}, + Group: "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") @@ -420,8 +207,8 @@ func TestPredefined404ServeHTTP(t *testing.T) { func TestGroupCertificate(t *testing.T) { testGroup := &Domain{ - group: Group{name: "group"}, - projectName: "", + Project: "", + Group: "group", } tls, err := testGroup.EnsureCertificate() @@ -431,11 +218,9 @@ func TestGroupCertificate(t *testing.T) { func TestDomainNoCertificate(t *testing.T) { testDomain := &Domain{ - group: Group{name: "group"}, - projectName: "project2", - config: &Config{ - Domain: "test.domain.com", - }, + Group: "group", + Project: "project2", + DomainName: "test.domain.com", } tls, err := testDomain.EnsureCertificate() @@ -449,13 +234,11 @@ func TestDomainNoCertificate(t *testing.T) { func TestDomainCertificate(t *testing.T) { testDomain := &Domain{ - group: Group{name: "group"}, - projectName: "project2", - config: &Config{ - Domain: "test.domain.com", - Certificate: fixture.Certificate, - Key: fixture.Key, - }, + Group: "group", + Project: "project2", + DomainName: "test.domain.com", + Certificate: fixture.Certificate, + Key: fixture.Key, } tls, err := testDomain.EnsureCertificate() @@ -463,37 +246,6 @@ func TestDomainCertificate(t *testing.T) { require.NoError(t, err) } -func TestCacheControlHeaders(t *testing.T) { - cleanup := setUpTests(t) - defer cleanup() - - testGroup := &Domain{ - group: Group{ - name: "group", - projects: map[string]*Project{ - "group.test.io": &Project{}, - }, - }, - } - w := httptest.NewRecorder() - req, err := http.NewRequest("GET", "http://group.test.io/", nil) - require.NoError(t, err) - - now := time.Now() - serveFileOrNotFound(testGroup)(w, req) - - require.Equal(t, http.StatusOK, w.Code) - require.Equal(t, "max-age=600", w.Header().Get("Cache-Control")) - - expires := w.Header().Get("Expires") - require.NotEmpty(t, expires) - - expiresTime, err := time.Parse(time.RFC1123, expires) - require.NoError(t, err) - - require.WithinDuration(t, now.UTC().Add(10*time.Minute), expiresTime.UTC(), time.Minute) -} - func TestOpenNoFollow(t *testing.T) { tmpfile, err := ioutil.TempFile("", "link-test") require.NoError(t, err) diff --git a/internal/domain/group.go b/internal/domain/group.go deleted file mode 100644 index e3424a4d0..000000000 --- a/internal/domain/group.go +++ /dev/null @@ -1,39 +0,0 @@ -package domain - -import ( - "path" - "strings" -) - -// Group represents a GitLab group with projects and subgroups -type Group struct { - name string - - // nested groups - subgroups subgroups - - // group domains: - projects projects -} - -type projects map[string]*Project -type subgroups map[string]*Group - -func (g *Group) digProjectWithSubpath(parentPath string, keys []string) (*Project, string, string) { - if len(keys) >= 1 { - head := keys[0] - tail := keys[1:] - currentPath := path.Join(parentPath, head) - search := strings.ToLower(head) - - if project := g.projects[search]; project != nil { - return project, currentPath, path.Join(tail...) - } - - if subgroup := g.subgroups[search]; subgroup != nil { - return subgroup.digProjectWithSubpath(currentPath, tail) - } - } - - return nil, "", "" -} diff --git a/internal/domain/project.go b/internal/domain/project.go deleted file mode 100644 index d0add00d5..000000000 --- a/internal/domain/project.go +++ /dev/null @@ -1,9 +0,0 @@ -package domain - -// Project represents GitLab project settings -type Project struct { - NamespaceProject bool - HTTPSOnly bool - AccessControl bool - ID uint64 -} diff --git a/internal/domain/config.go b/internal/source/dirs/config.go similarity index 54% rename from internal/domain/config.go rename to internal/source/dirs/config.go index 0836eedd9..b26da8a6d 100644 --- a/internal/domain/config.go +++ b/internal/source/dirs/config.go @@ -1,4 +1,4 @@ -package domain +package dirs import ( "encoding/json" @@ -7,8 +7,8 @@ import ( "strings" ) -// Config represents a custom domain config -type Config struct { +// DomainConfig represents a custom domain config +type DomainConfig struct { Domain string Certificate string Key string @@ -17,16 +17,24 @@ type Config struct { AccessControl bool `json:"access_control"` } -// MultiConfig represents a group of custom domain configs -type MultiConfig struct { - Domains []Config +// MultiDomainConfig represents a group of custom domain configs +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 { + NamespaceProject bool + HTTPSOnly bool + AccessControl bool + ID uint64 +} + // Valid validates a custom domain config for a root domain -func (c *Config) Valid(rootDomain string) bool { +func (c *DomainConfig) Valid(rootDomain string) bool { if c.Domain == "" { return false } @@ -37,13 +45,13 @@ func (c *Config) Valid(rootDomain string) bool { return !strings.HasSuffix(domain, rootDomain) } -func (c *MultiConfig) Read(group, project string) (err error) { +// Read reads a multi domain config and decodes it from a `config.json` +func (c *MultiDomainConfig) Read(group, project string) error { configFile, err := os.Open(filepath.Join(group, project, "config.json")) if err != nil { return err } defer configFile.Close() - err = json.NewDecoder(configFile).Decode(c) - return + return json.NewDecoder(configFile).Decode(c) } diff --git a/internal/domain/config_test.go b/internal/source/dirs/config_test.go similarity index 75% rename from internal/domain/config_test.go rename to internal/source/dirs/config_test.go index a78c72cf1..d2bef10cd 100644 --- a/internal/domain/config_test.go +++ b/internal/source/dirs/config_test.go @@ -1,4 +1,4 @@ -package domain +package dirs import ( "io/ioutil" @@ -14,25 +14,25 @@ const invalidConfig = `{"Domains":{}}` const validConfig = `{"Domains":[{"Domain":"test"}]}` func TestDomainConfigValidness(t *testing.T) { - d := Config{} + d := DomainConfig{} require.False(t, d.Valid("gitlab.io")) - d = Config{Domain: "test"} + d = DomainConfig{Domain: "test"} require.True(t, d.Valid("gitlab.io")) - d = Config{Domain: "test"} + d = DomainConfig{Domain: "test"} require.True(t, d.Valid("gitlab.io")) - d = Config{Domain: "test.gitlab.io"} + d = DomainConfig{Domain: "test.gitlab.io"} require.False(t, d.Valid("gitlab.io")) - d = Config{Domain: "test.test.gitlab.io"} + d = DomainConfig{Domain: "test.test.gitlab.io"} require.False(t, d.Valid("gitlab.io")) - d = Config{Domain: "test.testgitlab.io"} + d = DomainConfig{Domain: "test.testgitlab.io"} require.True(t, d.Valid("gitlab.io")) - d = Config{Domain: "test.GitLab.Io"} + d = DomainConfig{Domain: "test.GitLab.Io"} require.False(t, d.Valid("gitlab.io")) } @@ -40,26 +40,26 @@ func TestDomainConfigRead(t *testing.T) { cleanup := setUpTests(t) defer cleanup() - d := MultiConfig{} + 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 = MultiConfig{} + 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 = MultiConfig{} + 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 = MultiConfig{} + d = MultiDomainConfig{} err = d.Read("test-group", "test-project") require.NoError(t, err) } diff --git a/internal/source/dirs/group.go b/internal/source/dirs/group.go new file mode 100644 index 000000000..917159e4c --- /dev/null +++ b/internal/source/dirs/group.go @@ -0,0 +1,135 @@ +package dirs + +import ( + "errors" + "net/http" + "path" + "strings" + + "gitlab.com/gitlab-org/gitlab-pages/internal/host" +) + +const ( + subgroupScanLimit int = 21 + // maxProjectDepth is set to the maximum nested project depth in gitlab (21) plus 3. + // One for the project, one for the first empty element of the split (URL.Path starts with /), + // and one for the real file path + maxProjectDepth int = subgroupScanLimit + 3 +) + +// Group represents a GitLab group with project configs and subgroups +type Group struct { + name string + + // nested groups + subgroups subgroups + + // group domains: + projects projects +} + +type projects map[string]*ProjectConfig +type subgroups map[string]*Group + +func (g *Group) digProjectWithSubpath(parentPath string, keys []string) (*ProjectConfig, string, string) { + if len(keys) >= 1 { + head := keys[0] + tail := keys[1:] + currentPath := path.Join(parentPath, head) + search := strings.ToLower(head) + + if project := g.projects[search]; project != nil { + return project, currentPath, path.Join(tail...) + } + + if subgroup := g.subgroups[search]; subgroup != nil { + return subgroup.digProjectWithSubpath(currentPath, tail) + } + } + + return nil, "", "" +} + +// Look up a project inside the domain based on the host and path. Returns the +// project and its name (if applicable) +func (group *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) + if len(split) >= 2 { + projectConfig, projectPath, urlPath := group.digProjectWithSubpath("", split[1:]) + if projectConfig != nil { + return projectConfig, projectPath, urlPath + } + } + + // Since the URL doesn't specify a project (e.g. http://mydomain.gitlab.io), + // return the group project if it exists. + if host := host.FromRequest(r); host != "" { + if groupProject := group.projects[host]; groupProject != nil { + return groupProject, host, strings.Join(split[1:], "/") + } + } + + return nil, "", "" +} + +func (g *Group) IsHTTPSOnly(r *http.Request) (bool, error) { + project, _, _ := g.getProjectConfigWithSubpath(r) + + if project != nil { + return project.HTTPSOnly, nil + } + + return false, errors.New("project not found") +} + +func (g *Group) HasAccessControl(r *http.Request) (bool, error) { + project, _, _ := g.getProjectConfigWithSubpath(r) + + if project != nil { + return project.AccessControl, nil + } + + return false, errors.New("project not found") +} + +func (g *Group) IsNamespaceProject(r *http.Request) (bool, error) { + project, _, _ := g.getProjectConfigWithSubpath(r) + + if project != nil { + return project.NamespaceProject, nil + } + + return false, errors.New("project not found") +} + +func (g *Group) ProjectID(r *http.Request) (uint64, error) { + project, _, _ := g.getProjectConfigWithSubpath(r) + + if project != nil { + return project.ID, nil + } + + return 0, errors.New("project not found") +} + +func (g *Group) ProjectExists(r *http.Request) (bool, error) { + project, _, _ := g.getProjectConfigWithSubpath(r) + + if project != nil { + return true, nil + } + + return false, nil +} + +func (g *Group) ProjectWithSubpath(r *http.Request) (string, string, error) { + project, projectName, subPath := g.getProjectConfigWithSubpath(r) + + if project != nil { + return projectName, subPath, nil + } + + return "", "", errors.New("project not found") +} diff --git a/internal/source/dirs/group_domain_test.go b/internal/source/dirs/group_domain_test.go new file mode 100644 index 000000000..3c20549bb --- /dev/null +++ b/internal/source/dirs/group_domain_test.go @@ -0,0 +1,440 @@ +package dirs + +import ( + "compress/gzip" + "io/ioutil" + "net/http" + "net/http/httptest" + "net/url" + "os" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/fixture" + "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" +) + +func serveFileOrNotFound(domain *domain.Domain) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if !domain.ServeFileHTTP(w, r) { + domain.ServeNotFoundHTTP(w, r) + } + } +} + +func testGroupServeHTTPHost(t *testing.T, host string) { + testGroup := &domain.Domain{ + Project: "", + Group: "group", + GroupConfig: &Group{ + name: "group", + projects: map[string]*ProjectConfig{ + "group.test.io": &ProjectConfig{}, + "group.gitlab-example.com": &ProjectConfig{}, + "project": &ProjectConfig{}, + "project2": &ProjectConfig{}, + }, + }, + } + + makeURL := func(path string) string { + return "http://" + host + path + } + + serve := serveFileOrNotFound(testGroup) + + require.HTTPBodyContains(t, serve, "GET", makeURL("/"), nil, "main-dir") + require.HTTPBodyContains(t, serve, "GET", makeURL("/index"), nil, "main-dir") + require.HTTPBodyContains(t, serve, "GET", makeURL("/index.html"), nil, "main-dir") + testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project"), nil, "//"+host+"/project/") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project/"), nil, "project-subdir") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index"), nil, "project-subdir") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index/"), nil, "project-subdir") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index.html"), nil, "project-subdir") + testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project/subdir"), nil, "//"+host+"/project/subdir/") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project/subdir/"), nil, "project-subsubdir") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project2/"), nil, "project2-main") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project2/index"), nil, "project2-main") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project2/index.html"), nil, "project2-main") + require.HTTPError(t, serve, "GET", makeURL("/private.project/"), nil) + require.HTTPError(t, serve, "GET", makeURL("//about.gitlab.com/%2e%2e"), nil) + require.HTTPError(t, serve, "GET", makeURL("/symlink"), nil) + require.HTTPError(t, serve, "GET", makeURL("/symlink/index.html"), nil) + require.HTTPError(t, serve, "GET", makeURL("/symlink/subdir/"), nil) + require.HTTPError(t, serve, "GET", makeURL("/project/fifo"), nil) + require.HTTPError(t, serve, "GET", makeURL("/not-existing-file"), nil) + require.HTTPRedirect(t, serve, "GET", makeURL("/project//about.gitlab.com/%2e%2e"), nil) +} + +func TestGroupServeHTTP(t *testing.T) { + cleanup := setUpTests(t) + defer cleanup() + + t.Run("group.test.io", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io") }) + t.Run("group.test.io:8080", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io:8080") }) +} + +func TestDomainServeHTTP(t *testing.T) { + cleanup := setUpTests(t) + defer cleanup() + + testDomain := &domain.Domain{ + Group: "group", + Project: "project2", + 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 + domain *domain.Domain + url string + expected bool + }{ + { + name: "Default group domain with HTTPS-only enabled", + domain: &domain.Domain{ + Group: "group", + Project: "project", + GroupConfig: &Group{ + name: "group", + projects: projects{"test-domain": &ProjectConfig{HTTPSOnly: true}}, + }, + }, + url: "http://test-domain", + expected: true, + }, + { + name: "Default group domain with HTTPS-only disabled", + domain: &domain.Domain{ + Group: "group", + Project: "project", + GroupConfig: &Group{ + name: "group", + projects: projects{"test-domain": &ProjectConfig{HTTPSOnly: false}}, + }, + }, + url: "http://test-domain", + expected: false, + }, + { + name: "Case-insensitive default group domain with HTTPS-only enabled", + domain: &domain.Domain{ + Project: "project", + Group: "group", + GroupConfig: &Group{ + name: "group", + projects: projects{"test-domain": &ProjectConfig{HTTPSOnly: true}}, + }, + }, + url: "http://Test-domain", + expected: true, + }, + { + name: "Other group domain with HTTPS-only enabled", + domain: &domain.Domain{ + Project: "project", + Group: "group", + GroupConfig: &Group{ + name: "group", + projects: projects{"project": &ProjectConfig{HTTPSOnly: true}}, + }, + }, + url: "http://test-domain/project", + expected: true, + }, + { + name: "Other group domain with HTTPS-only disabled", + domain: &domain.Domain{ + Project: "project", + Group: "group", + GroupConfig: &Group{ + name: "group", + projects: projects{"project": &ProjectConfig{HTTPSOnly: false}}, + }, + }, + url: "http://test-domain/project", + expected: false, + }, + { + name: "Unknown project", + domain: &domain.Domain{ + Group: "group", + Project: "project", + }, + url: "http://test-domain/project", + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + req, _ := http.NewRequest(http.MethodGet, test.url, nil) + require.Equal(t, test.expected, test.domain.IsHTTPSOnly(req)) + }) + } +} + +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) + require.NoError(t, err) + if acceptEncoding != "" { + req.Header.Add("Accept-Encoding", acceptEncoding) + } + handler(w, req) + + if ungzip { + reader, err := gzip.NewReader(w.Body) + require.NoError(t, err) + defer reader.Close() + + contentEncoding := w.Header().Get("Content-Encoding") + require.Equal(t, "gzip", contentEncoding, "Content-Encoding") + + bytes, err := ioutil.ReadAll(reader) + require.NoError(t, err) + require.Contains(t, string(bytes), str) + } else { + require.Contains(t, w.Body.String(), str) + } + + require.Equal(t, contentType, w.Header().Get("Content-Type")) +} + +func TestGroupServeHTTPGzip(t *testing.T) { + cleanup := setUpTests(t) + defer cleanup() + + testGroup := &domain.Domain{ + Project: "", + Group: "group", + GroupConfig: &Group{ + name: "group", + projects: map[string]*ProjectConfig{ + "group.test.io": &ProjectConfig{}, + "group.gitlab-example.com": &ProjectConfig{}, + "project": &ProjectConfig{}, + "project2": &ProjectConfig{}, + }, + }, + } + + testSet := []struct { + mode string // HTTP mode + url string // Test URL + acceptEncoding string // Accept encoding header + body interface{} // Expected body at above URL + contentType string // Expected content-type + ungzip bool // Expect the response to be gzipped? + }{ + // No gzip encoding requested + {"GET", "/index.html", "", "main-dir", "text/html; charset=utf-8", false}, + {"GET", "/index.html", "identity", "main-dir", "text/html; charset=utf-8", false}, + {"GET", "/index.html", "gzip; q=0", "main-dir", "text/html; charset=utf-8", false}, + // gzip encoding requested, + {"GET", "/index.html", "*", "main-dir", "text/html; charset=utf-8", true}, + {"GET", "/index.html", "identity, gzip", "main-dir", "text/html; charset=utf-8", true}, + {"GET", "/index.html", "gzip", "main-dir", "text/html; charset=utf-8", true}, + {"GET", "/index.html", "gzip; q=1", "main-dir", "text/html; charset=utf-8", true}, + {"GET", "/index.html", "gzip; q=0.9", "main-dir", "text/html; charset=utf-8", true}, + {"GET", "/index.html", "gzip, deflate", "main-dir", "text/html; charset=utf-8", true}, + {"GET", "/index.html", "gzip; q=1, deflate", "main-dir", "text/html; charset=utf-8", true}, + {"GET", "/index.html", "gzip; q=0.9, deflate", "main-dir", "text/html; charset=utf-8", true}, + // gzip encoding requested, but url does not have compressed content on disk + {"GET", "/project2/index.html", "*", "project2-main", "text/html; charset=utf-8", false}, + {"GET", "/project2/index.html", "identity, gzip", "project2-main", "text/html; charset=utf-8", false}, + {"GET", "/project2/index.html", "gzip", "project2-main", "text/html; charset=utf-8", false}, + {"GET", "/project2/index.html", "gzip; q=1", "project2-main", "text/html; charset=utf-8", false}, + {"GET", "/project2/index.html", "gzip; q=0.9", "project2-main", "text/html; charset=utf-8", false}, + {"GET", "/project2/index.html", "gzip, deflate", "project2-main", "text/html; charset=utf-8", false}, + {"GET", "/project2/index.html", "gzip; q=1, deflate", "project2-main", "text/html; charset=utf-8", false}, + {"GET", "/project2/index.html", "gzip; q=0.9, deflate", "project2-main", "text/html; charset=utf-8", false}, + // malformed headers + {"GET", "/index.html", ";; gzip", "main-dir", "text/html; charset=utf-8", false}, + {"GET", "/index.html", "middle-out", "main-dir", "text/html; charset=utf-8", false}, + {"GET", "/index.html", "gzip; quality=1", "main-dir", "text/html; charset=utf-8", false}, + // Symlinked .gz files are not supported + {"GET", "/gz-symlink", "*", "data", "text/plain; charset=utf-8", false}, + // Unknown file-extension, with text content + {"GET", "/text.unknown", "*", "hello", "text/plain; charset=utf-8", true}, + {"GET", "/text-nogzip.unknown", "*", "hello", "text/plain; charset=utf-8", false}, + // Unknown file-extension, with PNG content + {"GET", "/image.unknown", "*", "GIF89a", "image/gif", true}, + {"GET", "/image-nogzip.unknown", "*", "GIF89a", "image/gif", false}, + } + + for _, tt := range testSet { + URL := "http://group.test.io" + tt.url + testHTTPGzip(t, serveFileOrNotFound(testGroup), tt.mode, URL, nil, tt.acceptEncoding, tt.body, tt.contentType, tt.ungzip) + } +} + +func TestGroup404ServeHTTP(t *testing.T) { + cleanup := setUpTests(t) + defer cleanup() + + testGroup := &domain.Domain{ + Project: "", + 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{}, + }, + }, + } + + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/not/existing-file", nil, "Custom 404 project page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/", nil, "Custom 404 project page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not/existing-file", nil, "Custom 404 group page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") + require.HTTPBodyNotContains(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404.symlink/not/existing-file", nil, "Custom 404 project page") + + // Ensure the namespace project's custom 404.html is not used by projects + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.no.404/not/existing-file", nil, "The page you're looking for could not be found.") +} + +func TestDomain404ServeHTTP(t *testing.T) { + cleanup := setUpTests(t) + defer cleanup() + + testDomain := &domain.Domain{ + Project: "domain.404", + Group: "group.404", + 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.Domain{ + Group: "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") +} + +func TestGroupCertificate(t *testing.T) { + testGroup := &domain.Domain{ + Group: "group", + Project: "", + } + + tls, err := testGroup.EnsureCertificate() + require.Nil(t, tls) + require.Error(t, err) +} + +func TestDomainNoCertificate(t *testing.T) { + testDomain := &domain.Domain{ + Group: "group", + Project: "project2", + DomainName: "test.domain.com", + } + + tls, err := testDomain.EnsureCertificate() + require.Nil(t, tls) + require.Error(t, err) + + _, err2 := testDomain.EnsureCertificate() + require.Error(t, err) + require.Equal(t, err, err2) +} + +func TestDomainCertificate(t *testing.T) { + testDomain := &domain.Domain{ + Project: "project2", + Group: "group", + DomainName: "test.domain.com", + Certificate: fixture.Certificate, + Key: fixture.Key, + } + + tls, err := testDomain.EnsureCertificate() + require.NotNil(t, tls) + require.NoError(t, err) +} + +func TestCacheControlHeaders(t *testing.T) { + cleanup := setUpTests(t) + defer cleanup() + + testGroup := &domain.Domain{ + Group: "group", + GroupConfig: &Group{ + name: "group", + projects: map[string]*ProjectConfig{ + "group.test.io": &ProjectConfig{}, + }, + }, + } + w := httptest.NewRecorder() + req, err := http.NewRequest("GET", "http://group.test.io/", nil) + require.NoError(t, err) + + now := time.Now() + serveFileOrNotFound(testGroup)(w, req) + + require.Equal(t, http.StatusOK, w.Code) + require.Equal(t, "max-age=600", w.Header().Get("Cache-Control")) + + expires := w.Header().Get("Expires") + require.NotEmpty(t, expires) + + expiresTime, err := time.Parse(time.RFC1123, expires) + require.NoError(t, err) + + require.WithinDuration(t, now.UTC().Add(10*time.Minute), expiresTime.UTC(), time.Minute) +} + +var chdirSet = false + +func setUpTests(t require.TestingT) func() { + return chdirInPath(t, "../../../shared/pages") +} + +func chdirInPath(t require.TestingT, path string) func() { + noOp := func() {} + if chdirSet { + return noOp + } + + cwd, err := os.Getwd() + require.NoError(t, err, "Cannot Getwd") + + err = os.Chdir(path) + require.NoError(t, err, "Cannot Chdir") + + chdirSet = true + return func() { + err := os.Chdir(cwd) + require.NoError(t, err, "Cannot Chdir in cleanup") + + chdirSet = false + } +} diff --git a/internal/domain/group_test.go b/internal/source/dirs/group_test.go similarity index 89% rename from internal/domain/group_test.go rename to internal/source/dirs/group_test.go index ac7afea10..8633e7e4c 100644 --- a/internal/domain/group_test.go +++ b/internal/source/dirs/group_test.go @@ -1,4 +1,4 @@ -package domain +package dirs import ( "strings" @@ -8,13 +8,13 @@ import ( ) func TestGroupDig(t *testing.T) { - matchingProject := &Project{ID: 1} + matchingProject := &ProjectConfig{ID: 1} tests := []struct { name string g Group path string - expectedProject *Project + 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": &Project{}}, + projects: projects{"another": &ProjectConfig{}}, }, }, }, @@ -66,7 +66,7 @@ func TestGroupDig(t *testing.T) { projects: projects{"projectb": matchingProject}, }, }, - projects: projects{"another": &Project{}}, + 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": &Project{}}, + projects: projects{"another": &ProjectConfig{}}, }, }, }, diff --git a/internal/domain/map.go b/internal/source/dirs/map.go similarity index 87% rename from internal/domain/map.go rename to internal/source/dirs/map.go index f65f52af4..849f9e149 100644 --- a/internal/domain/map.go +++ b/internal/source/dirs/map.go @@ -1,4 +1,4 @@ -package domain +package dirs import ( "bytes" @@ -12,33 +12,39 @@ import ( "github.com/karrick/godirwalk" log "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/metrics" ) // Map maps domain names to Domain instances. -type Map map[string]*Domain +type Map map[string]*domain.Domain type domainsUpdater func(Map) -func (dm Map) updateDomainMap(domainName string, domain *Domain) { +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.projectName, - "old_group": old.group, - "old_project_name": old.projectName, + "new_group": domain.Group, + "new_project_name": domain.Project, + "old_group": old.Group, + "old_project_name": old.Project, }).Error("Duplicate domain") } dm[domainName] = domain } -func (dm Map) addDomain(rootDomain, groupName, projectName string, config *Config) { - newDomain := &Domain{ - group: Group{name: groupName}, - projectName: projectName, - config: config, +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, } var domainName string @@ -51,8 +57,9 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https groupDomain := dm[domainName] if groupDomain == nil { - groupDomain = &Domain{ - group: Group{ + groupDomain = &domain.Domain{ + Group: groupName, + GroupConfig: &Group{ name: groupName, projects: make(projects), subgroups: make(subgroups), @@ -62,7 +69,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https split := strings.SplitN(strings.ToLower(projectPath), "/", maxProjectDepth) projectName := split[len(split)-1] - g := &groupDomain.group + g := groupDomain.GroupConfig.(*Group) for i := 0; i < len(split)-1; i++ { subgroupName := split[i] @@ -79,7 +86,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https g = subgroup } - g.projects[projectName] = &Project{ + g.projects[projectName] = &ProjectConfig{ NamespaceProject: domainName == projectName, HTTPSOnly: httpsOnly, AccessControl: accessControl, @@ -89,7 +96,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https dm[domainName] = groupDomain } -func (dm Map) readProjectConfig(rootDomain string, group, projectName string, config *MultiConfig) { +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 @@ -131,7 +138,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 := &MultiConfig{} + config := &MultiDomainConfig{} if err := config.Read(group, projectPath); err != nil { config = nil } @@ -163,7 +170,7 @@ func readProjects(group, parent string, level int, buf []byte, fanIn chan<- jobR type jobResult struct { group string project string - config *MultiConfig + config *MultiDomainConfig } // ReadGroups walks the pages directory and populates dm with all the domains it finds. diff --git a/internal/domain/map_test.go b/internal/source/dirs/map_test.go similarity index 94% rename from internal/domain/map_test.go rename to internal/source/dirs/map_test.go index f184672ce..ac9b14a99 100644 --- a/internal/domain/map_test.go +++ b/internal/source/dirs/map_test.go @@ -1,4 +1,4 @@ -package domain +package dirs import ( "crypto/rand" @@ -68,16 +68,15 @@ func TestReadProjects(t *testing.T) { } // Check that multiple domains in the same project are recorded faithfully - exp1 := &Config{Domain: "test.domain.com"} - require.Equal(t, exp1, dm["test.domain.com"].config) - - exp2 := &Config{Domain: "other.domain.com", Certificate: "test", Key: "key"} - require.Equal(t, exp2, dm["other.domain.com"].config) + 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) // check subgroups domain, ok := dm["group.test.io"] require.True(t, ok, "missing group.test.io domain") - subgroup, ok := domain.group.subgroups["subgroup"] + subgroup, ok := domain.GroupConfig.(*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") @@ -118,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.group + subgroup := domain.GroupConfig.(*Group) for i := 0; i < levels; i++ { subgroup, ok = subgroup.subgroups["sub"] if i <= subgroupScanLimit { -- GitLab From 602eb300822f23e65c8e961111d8b16096158285 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sun, 22 Sep 2019 12:25:27 +0200 Subject: [PATCH 05/13] Remove refactoring stubs and fix go lint --- internal/domain/domain.go | 57 +++++++---------------------------- internal/source/dirs/group.go | 46 +++++++++++++++++----------- 2 files changed, 39 insertions(+), 64 deletions(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 1c90a2322..4c5fce17d 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -29,12 +29,13 @@ type locationFileNoExtensionError struct { FullPath string } +// GroupConfig represents a per-request config for a group domain type GroupConfig interface { - IsHTTPSOnly(*http.Request) (bool, error) - HasAccessControl(*http.Request) (bool, error) - IsNamespaceProject(*http.Request) (bool, error) - ProjectID(*http.Request) (uint64, error) - ProjectExists(*http.Request) (bool, error) + 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) } @@ -123,15 +124,7 @@ func (d *Domain) IsHTTPSOnly(r *http.Request) bool { } // Check projects served under the group domain, including the default one - // TODO REFACTORING - // if project, _, _ := d.getProjectWithSubpath(r); project != nil { - // return project.HTTPSOnly - // } - if httpsOnly, err := d.GroupConfig.IsHTTPSOnly(r); err == nil { - return httpsOnly - } - - return false + return d.GroupConfig.IsHTTPSOnly(r) } // IsAccessControlEnabled figures out if the request is to a project that has access control enabled @@ -146,14 +139,7 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool { } // Check projects served under the group domain, including the default one - // TODO RFR if project, _, _ := d.getProjectWithSubpath(r); project != nil { - // return project.AccessControl - //} - if hasAccessControl, err := d.GroupConfig.HasAccessControl(r); err == nil { - return hasAccessControl - } - - return false + return d.GroupConfig.HasAccessControl(r) } // HasAcmeChallenge checks domain directory contains particular acme challenge @@ -195,14 +181,7 @@ func (d *Domain) IsNamespaceProject(r *http.Request) bool { } // Check projects served under the group domain, including the default one - // if project, _, _ := d.getProjectWithSubpath(r); project != nil { - // return project.NamespaceProject - // } - if isNamespaceProject, err := d.GroupConfig.IsNamespaceProject(r); err == nil { - return isNamespaceProject - } - - return false + return d.GroupConfig.IsNamespaceProject(r) } // GetID figures out what is the ID of the project user tries to access @@ -215,14 +194,7 @@ func (d *Domain) GetID(r *http.Request) uint64 { return d.ProjectID } - // if project, _, _ := d.getProjectWithSubpath(r); project != nil { - // return project.ID - // } - if projectID, err := d.GroupConfig.ProjectID(r); err == nil { - return projectID - } - - return 0 + return d.GroupConfig.ProjectID(r) } // HasProject figures out if the project exists that the user tries to access @@ -235,14 +207,7 @@ func (d *Domain) HasProject(r *http.Request) bool { return true } - // if project, _, _ := d.getProjectWithSubpath(r); project != nil { - // return true - // } - if projectExists, err := d.GroupConfig.ProjectExists(r); err == nil { - return projectExists - } - - return false + return d.GroupConfig.ProjectExists(r) } // Detect file's content-type either by extension or mime-sniffing. diff --git a/internal/source/dirs/group.go b/internal/source/dirs/group.go index 917159e4c..a711221c1 100644 --- a/internal/source/dirs/group.go +++ b/internal/source/dirs/group.go @@ -52,12 +52,12 @@ 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 (group *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) if len(split) >= 2 { - projectConfig, projectPath, urlPath := group.digProjectWithSubpath("", split[1:]) + projectConfig, projectPath, urlPath := g.digProjectWithSubpath("", split[1:]) if projectConfig != nil { return projectConfig, projectPath, urlPath } @@ -66,7 +66,7 @@ func (group *Group) getProjectConfigWithSubpath(r *http.Request) (*ProjectConfig // Since the URL doesn't specify a project (e.g. http://mydomain.gitlab.io), // return the group project if it exists. if host := host.FromRequest(r); host != "" { - if groupProject := group.projects[host]; groupProject != nil { + if groupProject := g.projects[host]; groupProject != nil { return groupProject, host, strings.Join(split[1:], "/") } } @@ -74,56 +74,66 @@ func (group *Group) getProjectConfigWithSubpath(r *http.Request) (*ProjectConfig return nil, "", "" } -func (g *Group) IsHTTPSOnly(r *http.Request) (bool, error) { +// IsHTTPSOnly return true if project exists and has https-only setting +// configured +func (g *Group) IsHTTPSOnly(r *http.Request) bool { project, _, _ := g.getProjectConfigWithSubpath(r) if project != nil { - return project.HTTPSOnly, nil + return project.HTTPSOnly } - return false, errors.New("project not found") + return false } -func (g *Group) HasAccessControl(r *http.Request) (bool, error) { +// 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, nil + return project.AccessControl } - return false, errors.New("project not found") + return false } -func (g *Group) IsNamespaceProject(r *http.Request) (bool, error) { +// 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, nil + return project.NamespaceProject } - return false, errors.New("project not found") + return false } -func (g *Group) ProjectID(r *http.Request) (uint64, error) { +// 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, nil + return project.ID } - return 0, errors.New("project not found") + return 0 } -func (g *Group) ProjectExists(r *http.Request) (bool, error) { +// 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, nil + return true } - return false, nil + 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) -- GitLab From 83376dd5016d918e579342d935c18ad2002c1dca Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 24 Sep 2019 12:12:11 +0200 Subject: [PATCH 06/13] Encapsulate groups config in the source package --- app.go | 36 ++++-------- internal/auth/auth.go | 24 ++++---- internal/auth/auth_test.go | 11 ++-- internal/source/domains.go | 56 +++++++++++++++++++ internal/source/{dirs => groups}/config.go | 2 +- .../source/{dirs => groups}/config_test.go | 2 +- internal/source/{dirs => groups}/group.go | 2 +- .../{dirs => groups}/group_domain_test.go | 2 +- .../source/{dirs => groups}/group_test.go | 2 +- internal/source/{dirs => groups}/map.go | 2 +- internal/source/{dirs => groups}/map_test.go | 2 +- internal/source/source.go | 14 ----- 12 files changed, 92 insertions(+), 63 deletions(-) create mode 100644 internal/source/domains.go rename internal/source/{dirs => groups}/config.go (98%) rename internal/source/{dirs => groups}/config_test.go (99%) rename internal/source/{dirs => groups}/group.go (99%) rename internal/source/{dirs => groups}/group_domain_test.go (99%) rename internal/source/{dirs => groups}/group_test.go (99%) rename internal/source/{dirs => groups}/map.go (99%) rename internal/source/{dirs => groups}/map_test.go (99%) delete mode 100644 internal/source/source.go diff --git a/app.go b/app.go index c14d7bc2a..7b3a191c9 100644 --- a/app.go +++ b/app.go @@ -7,9 +7,7 @@ import ( "net" "net/http" "os" - "strings" "sync" - "time" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/rs/cors" @@ -28,7 +26,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/logging" "gitlab.com/gitlab-org/gitlab-pages/internal/netutil" "gitlab.com/gitlab-org/gitlab-pages/internal/request" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/dirs" + "gitlab.com/gitlab-org/gitlab-pages/internal/source" ) const ( @@ -48,8 +46,7 @@ var ( type theApp struct { appConfig - dm dirs.Map - lock sync.RWMutex + domains *source.Domains Artifact *artifact.Artifact Auth *auth.Auth AcmeMiddleware *acme.Middleware @@ -57,15 +54,7 @@ type theApp struct { } func (a *theApp) isReady() bool { - return a.dm != nil -} - -func (a *theApp) domain(host string) *domain.Domain { - host = strings.ToLower(host) - a.lock.RLock() - defer a.lock.RUnlock() - domain, _ := a.dm[host] - return domain + return a.domains.Ready() } func (a *theApp) ServeTLS(ch *tls.ClientHelloInfo) (*tls.Certificate, error) { @@ -73,7 +62,7 @@ func (a *theApp) ServeTLS(ch *tls.ClientHelloInfo) (*tls.Certificate, error) { return nil, nil } - if domain := a.domain(ch.ServerName); domain != nil { + if domain := a.domains.GetDomain(ch.ServerName); domain != nil { tls, _ := domain.EnsureCertificate() return tls, nil } @@ -107,6 +96,10 @@ func (a *theApp) getHostAndDomain(r *http.Request) (host string, domain *domain. return host, a.domain(host) } +func (a *theApp) domain(host string) *domain.Domain { + return a.domains.GetDomain(host) +} + func (a *theApp) checkAuthenticationIfNotExists(domain *domain.Domain, w http.ResponseWriter, r *http.Request) bool { if domain == nil || !domain.HasProject(r) { @@ -206,7 +199,7 @@ func (a *theApp) acmeMiddleware(handler http.Handler) http.Handler { // authMiddleware handles authentication requests func (a *theApp) authMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if a.Auth.TryAuthenticate(w, r, a.dm, &a.lock) { + if a.Auth.TryAuthenticate(w, r, a.domains) { return } @@ -323,12 +316,6 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { return handler, nil } -func (a *theApp) UpdateDomains(dm dirs.Map) { - a.lock.Lock() - defer a.lock.Unlock() - a.dm = dm -} - func (a *theApp) Run() { var wg sync.WaitGroup @@ -367,7 +354,7 @@ func (a *theApp) Run() { a.listenAdminUnix(&wg) a.listenAdminHTTPS(&wg) - go dirs.Watch(a.Domain, a.UpdateDomains, time.Second) + a.domains.Watch(a.Domain) wg.Wait() } @@ -474,7 +461,8 @@ func (a *theApp) listenAdminHTTPS(wg *sync.WaitGroup) { } func runApp(config appConfig) { - a := theApp{appConfig: config} + a := theApp{appConfig: config, domains: new(source.Domains)} + err := logging.ConfigureLogging(a.LogFormat, a.LogVerbose) if err != nil { log.WithError(err).Fatal("Failed to initialize logging") diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 154d86dae..95a26250f 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -11,7 +11,6 @@ import ( "net/http" "net/url" "strings" - "sync" "time" "github.com/gorilla/securecookie" @@ -22,7 +21,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" "gitlab.com/gitlab-org/gitlab-pages/internal/request" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/dirs" + "gitlab.com/gitlab-org/gitlab-pages/internal/source" "golang.org/x/crypto/hkdf" ) @@ -108,7 +107,7 @@ func (a *Auth) checkSession(w http.ResponseWriter, r *http.Request) (*sessions.S } // TryAuthenticate tries to authenticate user and fetch access token if request is a callback to auth -func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, dm dirs.Map, lock *sync.RWMutex) bool { +func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, domains *source.Domains) bool { if a == nil { return false @@ -126,7 +125,7 @@ func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, dm dirs.M logRequest(r).Info("Receive OAuth authentication callback") - if a.handleProxyingAuth(session, w, r, dm, lock) { + if a.handleProxyingAuth(session, w, r, domains) { return true } @@ -200,16 +199,17 @@ func (a *Auth) checkAuthenticationResponse(session *sessions.Session, w http.Res http.Redirect(w, r, redirectURI, 302) } -func (a *Auth) domainAllowed(domain string, dm dirs.Map, lock *sync.RWMutex) bool { - lock.RLock() - defer lock.RUnlock() +func (a *Auth) domainAllowed(domain string, domains *source.Domains) bool { + domainConfigured := (domain == a.pagesDomain) || strings.HasSuffix("."+domain, a.pagesDomain) - domain = strings.ToLower(domain) - _, present := dm[domain] - return domain == a.pagesDomain || strings.HasSuffix("."+domain, a.pagesDomain) || present + if domainConfigured { + return true + } + + return domains.HasDomain(domain) } -func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, dm dirs.Map, lock *sync.RWMutex) bool { +func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, domains *source.Domains) bool { // If request is for authenticating via custom domain if shouldProxyAuth(r) { domain := r.URL.Query().Get("domain") @@ -228,7 +228,7 @@ func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWrit host = proxyurl.Host } - if !a.domainAllowed(host, dm, lock) { + if !a.domainAllowed(host, domains) { logRequest(r).WithField("domain", host).Warn("Domain is not configured") httperrors.Serve401(w) return true diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 8102a5d18..e8ff5e94f 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -5,14 +5,13 @@ import ( "net/http" "net/http/httptest" "net/url" - "sync" "testing" "github.com/gorilla/sessions" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/request" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/dirs" + "gitlab.com/gitlab-org/gitlab-pages/internal/source" ) func createAuth(t *testing.T) *Auth { @@ -55,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, make(dirs.Map), &sync.RWMutex{})) + require.Equal(t, false, auth.TryAuthenticate(result, r, new(source.Domains))) } func TestTryAuthenticateWithError(t *testing.T) { @@ -66,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, make(dirs.Map), &sync.RWMutex{})) + require.Equal(t, true, auth.TryAuthenticate(result, r, new(source.Domains))) require.Equal(t, 401, result.Code) } @@ -83,7 +82,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) { session.Values["state"] = "state" session.Save(r, result) - require.Equal(t, true, auth.TryAuthenticate(result, r, make(dirs.Map), &sync.RWMutex{})) + require.Equal(t, true, auth.TryAuthenticate(result, r, new(source.Domains))) require.Equal(t, 401, result.Code) } @@ -123,7 +122,7 @@ func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) { }) result := httptest.NewRecorder() - require.Equal(t, true, auth.TryAuthenticate(result, r, make(dirs.Map), &sync.RWMutex{})) + require.Equal(t, true, auth.TryAuthenticate(result, r, new(source.Domains))) 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 new file mode 100644 index 000000000..5e7f113cf --- /dev/null +++ b/internal/source/domains.go @@ -0,0 +1,56 @@ +package source + +import ( + "strings" + "sync" + "time" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/groups" +) + +// Domains struct represents a map of all domains supported by pages. It is +// currently reading them from disk. +type Domains struct { + dm groups.Map + lock sync.RWMutex +} + +// GetDomain returns a domain from the domains map +func (d *Domains) GetDomain(host string) *domain.Domain { + host = strings.ToLower(host) + d.lock.RLock() + defer d.lock.RUnlock() + domain, _ := d.dm[host] + + return domain +} + +// HasDomain checks for presence of a domain in the domains map +func (d *Domains) HasDomain(host string) bool { + d.lock.RLock() + defer d.lock.RUnlock() + + host = strings.ToLower(host) + _, isPresent := d.dm[host] + + return isPresent +} + +// Ready checks if the domains source is ready for work +func (d *Domains) Ready() bool { + return d.dm != nil +} + +// 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) +} + +func (d *Domains) updateDomains(dm groups.Map) { + d.lock.Lock() + defer d.lock.Unlock() + + d.dm = dm +} diff --git a/internal/source/dirs/config.go b/internal/source/groups/config.go similarity index 98% rename from internal/source/dirs/config.go rename to internal/source/groups/config.go index b26da8a6d..889472598 100644 --- a/internal/source/dirs/config.go +++ b/internal/source/groups/config.go @@ -1,4 +1,4 @@ -package dirs +package groups import ( "encoding/json" diff --git a/internal/source/dirs/config_test.go b/internal/source/groups/config_test.go similarity index 99% rename from internal/source/dirs/config_test.go rename to internal/source/groups/config_test.go index d2bef10cd..ed563b87f 100644 --- a/internal/source/dirs/config_test.go +++ b/internal/source/groups/config_test.go @@ -1,4 +1,4 @@ -package dirs +package groups import ( "io/ioutil" diff --git a/internal/source/dirs/group.go b/internal/source/groups/group.go similarity index 99% rename from internal/source/dirs/group.go rename to internal/source/groups/group.go index a711221c1..e5534cf69 100644 --- a/internal/source/dirs/group.go +++ b/internal/source/groups/group.go @@ -1,4 +1,4 @@ -package dirs +package groups import ( "errors" diff --git a/internal/source/dirs/group_domain_test.go b/internal/source/groups/group_domain_test.go similarity index 99% rename from internal/source/dirs/group_domain_test.go rename to internal/source/groups/group_domain_test.go index 3c20549bb..38225cf51 100644 --- a/internal/source/dirs/group_domain_test.go +++ b/internal/source/groups/group_domain_test.go @@ -1,4 +1,4 @@ -package dirs +package groups import ( "compress/gzip" diff --git a/internal/source/dirs/group_test.go b/internal/source/groups/group_test.go similarity index 99% rename from internal/source/dirs/group_test.go rename to internal/source/groups/group_test.go index 8633e7e4c..a8070c87c 100644 --- a/internal/source/dirs/group_test.go +++ b/internal/source/groups/group_test.go @@ -1,4 +1,4 @@ -package dirs +package groups import ( "strings" diff --git a/internal/source/dirs/map.go b/internal/source/groups/map.go similarity index 99% rename from internal/source/dirs/map.go rename to internal/source/groups/map.go index 849f9e149..c72b2fe76 100644 --- a/internal/source/dirs/map.go +++ b/internal/source/groups/map.go @@ -1,4 +1,4 @@ -package dirs +package groups import ( "bytes" diff --git a/internal/source/dirs/map_test.go b/internal/source/groups/map_test.go similarity index 99% rename from internal/source/dirs/map_test.go rename to internal/source/groups/map_test.go index ac9b14a99..4f333c9ae 100644 --- a/internal/source/dirs/map_test.go +++ b/internal/source/groups/map_test.go @@ -1,4 +1,4 @@ -package dirs +package groups import ( "crypto/rand" diff --git a/internal/source/source.go b/internal/source/source.go deleted file mode 100644 index 23dc72fe1..000000000 --- a/internal/source/source.go +++ /dev/null @@ -1,14 +0,0 @@ -package source - -import ( - "net/http" - - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" -) - -// Source represents a source of information about a domain. Whenever a request -// appears a concret implementation of a Source should find a domain that is -// needed to handle the request and serve pages -type Source interface { - GetDomain(*http.Request) *domain.Domain -} -- GitLab From e5d6997a68f323bc345928d14ac902ac506b4a67 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 24 Sep 2019 12:24:04 +0200 Subject: [PATCH 07/13] Remove refactoring support comments and improve app.go --- app.go | 2 +- internal/domain/domain.go | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app.go b/app.go index 7b3a191c9..0d69ac797 100644 --- a/app.go +++ b/app.go @@ -62,7 +62,7 @@ func (a *theApp) ServeTLS(ch *tls.ClientHelloInfo) (*tls.Certificate, error) { return nil, nil } - if domain := a.domains.GetDomain(ch.ServerName); domain != nil { + if domain := a.domain(ch.ServerName); domain != nil { tls, _ := domain.EnsureCertificate() return tls, nil } diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 4c5fce17d..f342bd72c 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -118,7 +118,6 @@ func (d *Domain) IsHTTPSOnly(r *http.Request) bool { } // Check custom domain config (e.g. http://example.com) - // if d.!= nil { if d.isCustomDomain() { return d.HTTPSOnly } @@ -389,7 +388,6 @@ func (d *Domain) tryFile(w http.ResponseWriter, r *http.Request, projectName str } func (d *Domain) serveFileFromGroup(w http.ResponseWriter, r *http.Request) bool { - // project, projectName, subPath := d.getProjectWithSubpath(r) projectName, subPath, err := d.GroupConfig.ProjectWithSubpath(r) if err != nil { httperrors.Serve404(w) @@ -404,7 +402,6 @@ func (d *Domain) serveFileFromGroup(w http.ResponseWriter, r *http.Request) bool } func (d *Domain) serveNotFoundFromGroup(w http.ResponseWriter, r *http.Request) { - // project, projectName, _ := d.getProjectWithSubpath(r) projectName, _, err := d.GroupConfig.ProjectWithSubpath(r) if err != nil { -- GitLab From 4bcff3f158475aaeed66714376fafdd101367b46 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 25 Sep 2019 12:41:09 +0200 Subject: [PATCH 08/13] Generalize project config on a domain level --- internal/domain/config.go | 11 +++ internal/domain/domain.go | 57 ++++++++------- internal/domain/domain_test.go | 78 +++++++++++---------- internal/source/groups/group_domain_test.go | 29 ++++---- internal/source/groups/map.go | 18 ++--- internal/source/groups/map_test.go | 8 +-- 6 files changed, 111 insertions(+), 90 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 f342bd72c..784f5d41a 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -44,14 +44,8 @@ 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 + GroupConfig GroupConfig // handles group domain config certificate *tls.Certificate certificateError error @@ -72,7 +66,19 @@ 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 } func (l *locationDirectoryError) Error() string { @@ -113,13 +119,13 @@ func handleGZip(w http.ResponseWriter, r *http.Request, fullPath string) string // 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 @@ -128,13 +134,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 @@ -143,11 +149,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 } @@ -169,7 +171,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 } @@ -185,12 +187,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) @@ -198,7 +200,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 } @@ -439,13 +441,16 @@ func (d *Domain) serveNotFoundFromConfig(w http.ResponseWriter, r *http.Request) // 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 } @@ -456,7 +461,7 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { // ServeFileHTTP implements http.Handler. 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 } @@ -470,7 +475,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { // ServeNotFoundHTTP implements http.Handler. 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 552b2f15d..d59b9ce39 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 c72b2fe76..e570eb26a 100644 --- a/internal/source/groups/map.go +++ b/internal/source/groups/map.go @@ -37,14 +37,16 @@ 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, + 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, + }, } 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 48ca1a7f90b4aea979d91c27fcc7861169d8028c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 25 Sep 2019 12:44:30 +0200 Subject: [PATCH 09/13] 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 e570eb26a..aee038130 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 508be9806c17d6ab8ca75104f831c7a7a9dc9a3a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 25 Sep 2019 12:46:42 +0200 Subject: [PATCH 10/13] Remove serving package that we do not use yet --- internal/serving/handler.go | 7 ------- internal/serving/serving.go | 9 --------- 2 files changed, 16 deletions(-) delete mode 100644 internal/serving/handler.go delete mode 100644 internal/serving/serving.go 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 deleted file mode 100644 index bc0bb61db..000000000 --- a/internal/serving/serving.go +++ /dev/null @@ -1,9 +0,0 @@ -package serving - -import "net/http" - -// Serving represents an interface used to serve pages for a given domain / -// address -type Serving interface { - ServeHTTP(http.ResponseWriter, *http.Request) -} -- GitLab From 8e11c9aa018c157966f30a4d809c7ca454d310fa Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 25 Sep 2019 12:52:55 +0200 Subject: [PATCH 11/13] Do not export disk source internal config types --- internal/source/disk/config.go | 12 +++--- internal/source/disk/config_test.go | 22 +++++------ 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, 53 insertions(+), 53 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..1bb2364ab 100644 --- a/internal/source/disk/config_test.go +++ b/internal/source/disk/config_test.go @@ -14,25 +14,25 @@ const invalidConfig = `{"Domains":{}}` const validConfig = `{"Domains":[{"Domain":"test"}]}` func TestDomainConfigValidness(t *testing.T) { - d := DomainConfig{} + 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")) } @@ -40,26 +40,26 @@ 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 aee038130..2a6ada2d6 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -35,7 +35,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, @@ -88,7 +88,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, @@ -98,7 +98,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 @@ -140,7 +140,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 } @@ -172,7 +172,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 9beee9f6022a745a40537c0b5d26dc8a43d35944 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 27 Sep 2019 11:07:16 +0200 Subject: [PATCH 12/13] 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 f56d97f90b9bb67a242a1811bc6efa3592ac9f8a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 2 Oct 2019 09:27:37 +0200 Subject: [PATCH 13/13] Use more idiomatic way to initialize a struct in domains.go --- internal/source/domains.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index 85646b8a0..cd2d89c55 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -21,7 +21,7 @@ type Domains struct { // nil value. func NewDomains() *Domains { return &Domains{ - lock: new(sync.RWMutex), + lock: &sync.RWMutex{}, } } -- GitLab