Get namespace domain if auth fails for a private domain

Add acceptance test and some more domains for testing

Move namespace domain serving logic

Restore go.sum

Remove redundant return

Fix linter

5 files changed, 128 insertions, 37 deletions

diff --git a/internal/auth/auth.go b/internal/auth/auth.go
index c12207ca..768e7f75 100644
--- a/internal/auth/auth.go
+++ b/internal/auth/auth.go
@@ -18,7 +18,6 @@ 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"
@@ -437,10 +436,10 @@ func (a *Auth) IsAuthSupported() bool {
 	return a != nil
 }
 
-func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, projectID uint64) bool {
+func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, projectID uint64) (contentServed, authFailed bool) {
 	session := a.checkSessionIsValid(w, r)
 	if session == nil {
-		return true
+		return true, false
 	}
 
 	// Access token exists, authorize request
@@ -457,14 +456,14 @@ func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, proje
 		errortracking.Capture(err, errortracking.WithRequest(req))
 
 		httperrors.Serve500(w)
-		return true
+		return true, false
 	}
 
 	req.Header.Add("Authorization", "Bearer "+session.Values["access_token"].(string))
 	resp, err := a.apiClient.Do(req)
 
 	if err == nil && checkResponseForInvalidToken(resp, session, w, r) {
-		return true
+		return true, false
 	}
 
 	if err != nil || resp.StatusCode != 200 {
@@ -472,17 +471,17 @@ func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, proje
 			logRequest(r).WithError(err).Error("Failed to retrieve info with token")
 		}
 
-		return false
+		return false, true
 	}
 
-	return false
+	return false, false
 }
 
 // CheckAuthenticationWithoutProject checks if user is authenticated and has a valid token
-func (a *Auth) CheckAuthenticationWithoutProject(w http.ResponseWriter, r *http.Request) bool {
+func (a *Auth) CheckAuthenticationWithoutProject(w http.ResponseWriter, r *http.Request) (contentServed, authFailed bool) {
 	if a == nil {
 		// No auth supported
-		return false
+		return false, false
 	}
 
 	return a.checkAuthentication(w, r, 0)
@@ -512,7 +511,8 @@ func (a *Auth) RequireAuth(w http.ResponseWriter, r *http.Request) bool {
 }
 
 // CheckAuthentication checks if user is authenticated and has access to the project
-func (a *Auth) CheckAuthentication(w http.ResponseWriter, r *http.Request, domain *domain.Domain) bool {
+// will return contentServed = false when authFailed = true
+func (a *Auth) CheckAuthentication(w http.ResponseWriter, r *http.Request, projectID uint64) (contentServed, authFailed bool) {
 	logRequest(r).Debug("Authenticate request")
 
 	if a == nil {
@@ -520,29 +520,10 @@ func (a *Auth) CheckAuthentication(w http.ResponseWriter, r *http.Request, domai
 		errortracking.Capture(errAuthNotConfigured, errortracking.WithRequest(r))
 
 		httperrors.Serve500(w)
-		return true
-	}
-
-	if a.checkAuthentication(w, r, domain.GetProjectID(r)) {
-		// if auth fails, try to resolve parent namespace domain
-		r.URL.Path = "/"
-		parent, err := domain.Resolver.Resolve(r)
-		if err != nil {
-			httperrors.Serve404(w)
-			return true
-		}
-
-		// for namespace domains that have no access control enabled
-		if parent.LookupPath.IsNamespaceProject && !parent.LookupPath.HasAccessControl {
-			parent.ServeNotFoundHTTP(w, r)
-			return true
-		}
-
-		httperrors.Serve404(w)
-		return true
+		return true, false
 	}
 
-	return false
+	return a.checkAuthentication(w, r, projectID)
 }
 
 // CheckResponseForInvalidToken checks response for invalid token and destroys session if it was invalid
diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go
index fc8ddb44..711dea78 100644
--- a/internal/auth/auth_test.go
+++ b/internal/auth/auth_test.go
@@ -181,7 +181,10 @@ func TestCheckAuthenticationWhenAccess(t *testing.T) {
 	session.Values["access_token"] = "abc"
 	session.Save(r, result)
 
-	require.Equal(t, false, auth.CheckAuthentication(result, r, 1000))
+	contentServed, authFailed := auth.CheckAuthentication(result, r, 1000)
+	require.False(t, contentServed)
+	require.False(t, authFailed)
+	// content wasn't served so the default response from CheckAuthentication should be 200
 	require.Equal(t, 200, result.Code)
 }
 
@@ -219,8 +222,11 @@ func TestCheckAuthenticationWhenNoAccess(t *testing.T) {
 	session.Values["access_token"] = "abc"
 	session.Save(r, result)
 
-	require.Equal(t, true, auth.CheckAuthentication(result, r, 1000))
-	require.Equal(t, 404, result.Code)
+	contentServed, authFailed := auth.CheckAuthentication(result, r, 1000)
+	require.False(t, contentServed)
+	require.True(t, authFailed)
+	// content wasn't served so the default response from CheckAuthentication should be 200
+	require.Equal(t, 200, result.Code)
 }
 
 func TestCheckAuthenticationWhenInvalidToken(t *testing.T) {
@@ -257,7 +263,9 @@ func TestCheckAuthenticationWhenInvalidToken(t *testing.T) {
 	session.Values["access_token"] = "abc"
 	session.Save(r, result)
 
-	require.Equal(t, true, auth.CheckAuthentication(result, r, 1000))
+	contentServed, authFailed := auth.CheckAuthentication(result, r, 1000)
+	require.True(t, contentServed)
+	require.False(t, authFailed)
 	require.Equal(t, 302, result.Code)
 }
 
@@ -295,7 +303,9 @@ func TestCheckAuthenticationWithoutProject(t *testing.T) {
 	session.Values["access_token"] = "abc"
 	session.Save(r, result)
 
-	require.Equal(t, false, auth.CheckAuthenticationWithoutProject(result, r))
+	contentServed, authFailed := auth.CheckAuthenticationWithoutProject(result, r)
+	require.False(t, contentServed)
+	require.False(t, authFailed)
 	require.Equal(t, 200, result.Code)
 }
 
@@ -332,7 +342,9 @@ func TestCheckAuthenticationWithoutProjectWhenInvalidToken(t *testing.T) {
 	session.Values["access_token"] = "abc"
 	session.Save(r, result)
 
-	require.Equal(t, true, auth.CheckAuthenticationWithoutProject(result, r))
+	contentServed, authFailed := auth.CheckAuthenticationWithoutProject(result, r)
+	require.True(t, contentServed)
+	require.False(t, authFailed)
 	require.Equal(t, 302, result.Code)
 }
 
diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index 17a0e1d3..2a301121 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -168,3 +168,24 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 
 	request.ServeNotFoundHTTP(w, r)
 }
+
+// ServeNamespaceNotFound will try to find a parent namespace domain for a 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 nd try to resolve the domain name
+	r.URL.Path = "/"
+	namespaceDomain, err := d.Resolver.Resolve(r)
+	if err != nil {
+		httperrors.Serve404(w)
+		return
+	}
+
+	// for namespace domains that have no access control enabled
+	if namespaceDomain.LookupPath.IsNamespaceProject && !namespaceDomain.LookupPath.HasAccessControl {
+		namespaceDomain.ServeNotFoundHTTP(w, r)
+		return
+	}
+
+	httperrors.Serve404(w)
+}
diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go
index 49d46cb3..e152b031 100644
--- a/internal/domain/domain_test.go
+++ b/internal/domain/domain_test.go
@@ -1,7 +1,10 @@
 package domain
 
 import (
+	"fmt"
+	"io/ioutil"
 	"net/http"
+	"net/http/httptest"
 	"os"
 	"testing"
 
@@ -153,3 +156,76 @@ func chdirInPath(t require.TestingT, path string) func() {
 		chdirSet = false
 	}
 }
+
+func TestDomain_ServeNamespaceNotFound(t *testing.T) {
+	// defaultNotFound := "The page you're looking for could not be found."
+	// customNotFound := "Custom error page"
+
+	tests := []struct {
+		name             string
+		domain           string
+		path             string
+		resolver         *stubbedResolver
+		expectedResponse string
+	}{
+		{
+			name:   "public_namespace_domain",
+			domain: "group.404.gitlab-example.com",
+			path:   "/unknown",
+			resolver: &stubbedResolver{
+				project: &serving.LookupPath{
+					Path:               "../../shared/pages/group.404/group.404.gitlab-example.com/public",
+					IsNamespaceProject: true,
+				},
+				subpath: "/unknown",
+			},
+			expectedResponse: "Custom 404 group page",
+		},
+		{
+			name:   "private_project_under_public_namespace_domain",
+			domain: "group.404.gitlab-example.com",
+			path:   "/private_project/unknown",
+			resolver: &stubbedResolver{
+				project: &serving.LookupPath{
+					Path:               "../../shared/pages/group.404/group.404.gitlab-example.com/public",
+					IsNamespaceProject: true,
+					HasAccessControl:   false,
+				},
+				subpath: "/",
+			},
+			expectedResponse: "Custom 404 group page",
+		},
+		{
+			name:   "private_namespace_domain",
+			domain: "group.404.gitlab-example.com",
+			path:   "/unknown",
+			resolver: &stubbedResolver{
+				project: &serving.LookupPath{
+					Path:               "../../shared/pages/group.404/group.404.gitlab-example.com/public",
+					IsNamespaceProject: true,
+					HasAccessControl:   true,
+				},
+				subpath: "/",
+			},
+			expectedResponse: "The page you're looking for could not be found.",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			d := &Domain{
+				Name:     tt.domain,
+				Resolver: tt.resolver,
+			}
+			w := httptest.NewRecorder()
+			r := httptest.NewRequest("GET", fmt.Sprintf("http://%s%s", tt.domain, tt.path), nil)
+			d.ServeNamespaceNotFound(w, r)
+
+			resp := w.Result()
+			require.Equal(t, http.StatusNotFound, resp.StatusCode)
+			body, err := ioutil.ReadAll(resp.Body)
+			require.NoError(t, err)
+
+			require.Contains(t, string(body), tt.expectedResponse)
+		})
+	}
+}
diff --git a/internal/source/disk/map_test.go b/internal/source/disk/map_test.go
index d904e6aa..d2162883 100644
--- a/internal/source/disk/map_test.go
+++ b/internal/source/disk/map_test.go
@@ -49,6 +49,7 @@ func TestReadProjects(t *testing.T) {
 		"group.acme.test.io",
 		"withacmechallenge.domain.com",
 		"capitalgroup.test.io",
+		"group.404.gitlab-example.com",
 	}
 
 	for _, expected := range domains {