Address MR feedback

use correct reference

7 files changed, 66 insertions, 41 deletions

diff --git a/acceptance_test.go b/acceptance_test.go
index 2036c263..2b0a7800 100644
--- a/acceptance_test.go
+++ b/acceptance_test.go
@@ -1228,17 +1228,29 @@ func TestCustomErrorPageWithAuth(t *testing.T) {
 			expectedErrorPage: "Private custom 404 error page",
 		},
 		{
-			name:              "public_namespace_with_private_unauthorized_project",
-			domain:            "group.404.gitlab-example.com",
+			name:   "public_namespace_with_private_unauthorized_project",
+			domain: "group.404.gitlab-example.com",
+			// /private_unauthorized/config.json resolves project ID to 2000 which will cause a 401 from the mock GitLab testServer
 			path:              "/private_unauthorized/unknown",
 			expectedErrorPage: "Custom 404 group page",
 		},
+		{
+			name:              "private_namespace_authorized",
+			domain:            "group.auth.gitlab-example.com",
+			path:              "/unknown",
+			expectedErrorPage: "group.auth.gitlab-example.com namespace custom 404",
+		},
+		{
+			name:   "private_namespace_with_private_project_auth_failed",
+			domain: "group.auth.gitlab-example.com",
+			// project ID is 2000
+			path:              "/private.project.1/unknown",
+			expectedErrorPage: "The page you're looking for could not be found.",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			domain := tt.domain
-			path := tt.path
-			rsp, err := GetRedirectPage(t, httpListener, domain, path)
+			rsp, err := GetRedirectPage(t, httpListener, tt.domain, tt.path)
 			require.NoError(t, err)
 			defer rsp.Body.Close()
 
@@ -1248,7 +1260,7 @@ func TestCustomErrorPageWithAuth(t *testing.T) {
 			require.NoError(t, err)
 
 			state := url.Query().Get("state")
-			require.Equal(t, "http://"+domain, url.Query().Get("domain"))
+			require.Equal(t, "http://"+tt.domain, url.Query().Get("domain"))
 
 			pagesrsp, err := GetRedirectPage(t, httpListener, url.Host, url.Path+"?"+url.RawQuery)
 			require.NoError(t, err)
@@ -1267,12 +1279,12 @@ func TestCustomErrorPageWithAuth(t *testing.T) {
 			require.NoError(t, err)
 
 			// Will redirect to custom domain
-			require.Equal(t, domain, url.Host)
+			require.Equal(t, tt.domain, url.Host)
 			require.Equal(t, "1", url.Query().Get("code"))
 			require.Equal(t, state, url.Query().Get("state"))
 
 			// Run auth callback in custom domain
-			authrsp, err = GetRedirectPageWithCookie(t, httpListener, domain, "/auth?code=1&state="+
+			authrsp, err = GetRedirectPageWithCookie(t, httpListener, tt.domain, "/auth?code=1&state="+
 				state, cookie)
 
 			require.NoError(t, err)
@@ -1286,10 +1298,10 @@ func TestCustomErrorPageWithAuth(t *testing.T) {
 			require.NoError(t, err)
 
 			// Will redirect to custom domain error page
-			require.Equal(t, "http://"+domain+path, url.String())
+			require.Equal(t, "http://"+tt.domain+tt.path, url.String())
 
 			// Fetch page in custom domain
-			anotherResp, err := GetRedirectPageWithCookie(t, httpListener, domain, path, groupCookie)
+			anotherResp, err := GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path, groupCookie)
 			require.NoError(t, err)
 
 			require.Equal(t, http.StatusNotFound, anotherResp.StatusCode)
@@ -1372,7 +1384,7 @@ func TestAccessControlGroupDomain404RedirectsAuth(t *testing.T) {
 	teardown := RunPagesProcessWithAuth(t, *pagesBinary, listeners, "")
 	defer teardown()
 
-	rsp, err := GetRedirectPage(t, httpListener, "group.auth.gitlab-example.com", "/nonexistent/")
+	rsp, err := GetRedirectPage(t, httpListener, "group.gitlab-example.com", "/nonexistent/")
 	require.NoError(t, err)
 	defer rsp.Body.Close()
 	require.Equal(t, http.StatusFound, rsp.StatusCode)
diff --git a/app.go b/app.go
index 4ad4ffa3..5a195396 100644
--- a/app.go
+++ b/app.go
@@ -94,21 +94,18 @@ func (a *theApp) domain(host string) (*domain.Domain, error) {
 	return a.domains.GetDomain(host)
 }
 
-func (a *theApp) checkAuthenticationIfNotExists(domain *domain.Domain, w http.ResponseWriter, r *http.Request) bool {
-	if !domain.HasLookupPath(r) {
-		// Only if auth is supported
-		if a.Auth.IsAuthSupported() {
-			// To avoid user knowing if pages exist, we will force user to login and authorize pages
-			if a.Auth.CheckAuthenticationWithoutProject(w, r, domain) {
-				return true
-			}
-		}
-
-		domain.ServeNotFoundAuthFailed(w, r)
+// checkAuthAndServeNotFound performs the auth process if domain can't be found
+// the main purpose of this process is to avoid leaking the project existence/not-existence
+// by behaving the same if user has no access to the project or if project simply does not exists
+func (a *theApp) checkAuthAndServeNotFound(domain *domain.Domain, w http.ResponseWriter, r *http.Request) bool {
+	// To avoid user knowing if pages exist, we will force user to login and authorize pages
+	if a.Auth.CheckAuthenticationWithoutProject(w, r, domain) {
 		return true
 	}
 
-	return false
+	// auth succeeded try to serve the correct 404 page
+	domain.ServeNotFoundAuthFailed(w, r)
+	return true
 }
 
 func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, https bool, host string, domain *domain.Domain) bool {
@@ -127,8 +124,11 @@ func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, ht
 		return true
 	}
 
-	if a.checkAuthenticationIfNotExists(domain, w, r) {
-		return true
+	if !domain.HasLookupPath(r) {
+		// redirect to auth and serve not found
+		if a.checkAuthAndServeNotFound(domain, w, r) {
+			return true
+		}
 	}
 
 	if !https && domain.IsHTTPSOnly(r) {
diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go
index ffaf5b6f..39a533b3 100644
--- a/internal/auth/auth_test.go
+++ b/internal/auth/auth_test.go
@@ -30,7 +30,8 @@ func defaultCookieStore() sessions.Store {
 }
 
 type domainMock struct {
-	projectID uint64
+	projectID       uint64
+	notFoundContent string
 }
 
 func (dm *domainMock) GetProjectID(r *http.Request) uint64 {
@@ -39,6 +40,7 @@ func (dm *domainMock) GetProjectID(r *http.Request) uint64 {
 
 func (dm *domainMock) ServeNotFoundAuthFailed(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(http.StatusNotFound)
+	w.Write([]byte(dm.notFoundContent))
 }
 
 // Gorilla's sessions use request context to save session
@@ -195,7 +197,7 @@ func TestCheckAuthenticationWhenAccess(t *testing.T) {
 	contentServed := auth.CheckAuthentication(result, r, &domainMock{projectID: 1000})
 	require.False(t, contentServed)
 
-	// content wasn't served so the default response from CheckAuthentication should be 200
+	// notFoundContent wasn't served so the default response from CheckAuthentication should be 200
 	require.Equal(t, 200, result.Code)
 }
 
@@ -223,7 +225,8 @@ func TestCheckAuthenticationWhenNoAccess(t *testing.T) {
 		"http://pages.gitlab-example.com/auth",
 		apiServer.URL)
 
-	result := httptest.NewRecorder()
+	w := httptest.NewRecorder()
+
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
 	reqURL.Scheme = request.SchemeHTTPS
@@ -231,12 +234,18 @@ func TestCheckAuthenticationWhenNoAccess(t *testing.T) {
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
-	session.Save(r, result)
+	session.Save(r, w)
 
-	contentServed := auth.CheckAuthentication(result, r, &domainMock{projectID: 1000})
+	contentServed := auth.CheckAuthentication(w, r, &domainMock{projectID: 1000, notFoundContent: "Generic 404"})
 	require.True(t, contentServed)
-	// content wasn't served so the default response from CheckAuthentication should be 200
-	require.Equal(t, 404, result.Code)
+	res := w.Result()
+	defer res.Body.Close()
+
+	require.Equal(t, 404, res.StatusCode)
+
+	body, err := ioutil.ReadAll(res.Body)
+	require.NoError(t, err)
+	require.Equal(t, string(body), "Generic 404")
 }
 
 func TestCheckAuthenticationWhenInvalidToken(t *testing.T) {
diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index 9fa843a3..7c1639a3 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -1,6 +1,7 @@
 package domain
 
 import (
+	"context"
 	"crypto/tls"
 	"errors"
 	"net/http"
@@ -173,16 +174,18 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 // that failed authentication so that we serve the custom namespace error page for
 // public namespace domains
 func (d *Domain) serveNamespaceNotFound(w http.ResponseWriter, r *http.Request) {
-	// override the path and try to resolve the domain name
-	r.URL.Path = "/"
-	namespaceDomain, err := d.Resolver.Resolve(r)
+	// clone r and override the path and try to resolve the domain name
+	clonedReq := r.Clone(context.Background())
+	clonedReq.URL.Path = "/"
+
+	namespaceDomain, err := d.Resolver.Resolve(clonedReq)
 	if err != nil || namespaceDomain.LookupPath == nil {
 		httperrors.Serve404(w)
 		return
 	}
 
 	// for namespace domains that have no access control enabled
-	if namespaceDomain.LookupPath.IsNamespaceProject && !namespaceDomain.LookupPath.HasAccessControl {
+	if !namespaceDomain.LookupPath.HasAccessControl {
 		namespaceDomain.ServeNotFoundHTTP(w, r)
 		return
 	}
@@ -197,7 +200,7 @@ func (d *Domain) ServeNotFoundAuthFailed(w http.ResponseWriter, r *http.Request)
 		httperrors.Serve404(w)
 		return
 	}
-	if d.IsNamespaceProject(r) {
+	if d.IsNamespaceProject(r) && !d.GetLookupPath(r).HasAccessControl {
 		d.ServeNotFoundHTTP(w, r)
 		return
 	}
diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go
index 9e89f0e5..fc5611ba 100644
--- a/internal/domain/domain_test.go
+++ b/internal/domain/domain_test.go
@@ -157,10 +157,7 @@ func chdirInPath(t require.TestingT, path string) func() {
 	}
 }
 
-func TestDomain_ServeNamespaceNotFound(t *testing.T) {
-	// defaultNotFound := "The page you're looking for could not be found."
-	// customNotFound := "Custom error page"
-
+func TestServeNamespaceNotFound(t *testing.T) {
 	tests := []struct {
 		name             string
 		domain           string
@@ -231,6 +228,8 @@ func TestDomain_ServeNamespaceNotFound(t *testing.T) {
 			d.serveNamespaceNotFound(w, r)
 
 			resp := w.Result()
+			defer resp.Body.Close()
+
 			require.Equal(t, http.StatusNotFound, resp.StatusCode)
 			body, err := ioutil.ReadAll(resp.Body)
 			require.NoError(t, err)
diff --git a/shared/pages/group.auth/group.auth.gitlab-example.com/public/404.html b/shared/pages/group.auth/group.auth.gitlab-example.com/public/404.html
new file mode 100644
index 00000000..f345e8bc
--- /dev/null
+++ b/shared/pages/group.auth/group.auth.gitlab-example.com/public/404.html
@@ -0,0 +1 @@
+group.auth.gitlab-example.com namespace custom 404
diff --git a/shared/pages/group.auth/private.project.1/public/404.html b/shared/pages/group.auth/private.project.1/public/404.html
new file mode 100644
index 00000000..3b751385
--- /dev/null
+++ b/shared/pages/group.auth/private.project.1/public/404.html
@@ -0,0 +1 @@
+group.auth.gitlab-example.com/private.project.1 custom 404