Simplify domain and resolver checks

Return domain not found

Returns ErrDomainDoesNotExist when the lookup path
cannot be found for a specific project.

Closes https://gitlab.com/gitlab-org/gitlab-pages/issues/353

Return ErrDomainDoesNotExist in group resolver

2 files changed, 28 insertions, 46 deletions

diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index 5f32df0b..636b1bbd 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -11,9 +11,12 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
+// ErrDomainDoesNotExist returned when a domain is not found or when a lookup path
+// for a domain could not be resolved
+var ErrDomainDoesNotExist = errors.New("domain does not exist")
+
 // Domain is a domain that gitlab-pages can serve.
 type Domain struct {
 	Name            string
@@ -42,16 +45,9 @@ func (d *Domain) String() string {
 	return d.Name
 }
 
-func (d *Domain) isUnconfigured() bool {
-	if d == nil {
-		return true
-	}
-
-	return d.Resolver == nil
-}
 func (d *Domain) resolve(r *http.Request) (*serving.Request, error) {
-	if d.Resolver == nil {
-		return nil, client.ErrDomainDoesNotExist
+	if d == nil {
+		return nil, ErrDomainDoesNotExist
 	}
 
 	return d.Resolver.Resolve(r)
@@ -60,10 +56,6 @@ func (d *Domain) resolve(r *http.Request) (*serving.Request, error) {
 // GetLookupPath returns a project details based on the request. It returns nil
 // if project does not exist.
 func (d *Domain) GetLookupPath(r *http.Request) (*serving.LookupPath, error) {
-	if d.isUnconfigured() {
-		return nil, errors.New("not configured")
-	}
-
 	servingReq, err := d.resolve(r)
 	if err != nil {
 		return nil, err
@@ -111,16 +103,17 @@ func (d *Domain) GetProjectID(r *http.Request) uint64 {
 
 // HasLookupPath figures out if the project exists that the user tries to access
 func (d *Domain) HasLookupPath(r *http.Request) bool {
-	if _, err := d.GetLookupPath(r); err != nil {
+	if d == nil {
 		return false
 	}
+	_, err := d.GetLookupPath(r)
 
-	return true
+	return err == nil
 }
 
 // EnsureCertificate parses the PEM-encoded certificate for the domain
 func (d *Domain) EnsureCertificate() (*tls.Certificate, error) {
-	if d.isUnconfigured() || len(d.CertificateKey) == 0 || len(d.CertificateCert) == 0 {
+	if d == nil || len(d.CertificateKey) == 0 || len(d.CertificateCert) == 0 {
 		return nil, errors.New("tls certificates can be loaded only for pages with configuration")
 	}
 
@@ -142,7 +135,7 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) {
 func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 	request, err := d.resolve(r)
 	if err != nil {
-		if errors.Is(err, client.ErrDomainDoesNotExist) {
+		if errors.Is(err, ErrDomainDoesNotExist) {
 			// serve generic 404
 			httperrors.Serve404(w)
 			return true
@@ -153,11 +146,6 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 		return true
 	}
 
-	if request.LookupPath == nil {
-		// return and let ServeNotFoundHTTP serve the 404 page
-		return false
-	}
-
 	return request.ServeFileHTTP(w, r)
 }
 
@@ -165,7 +153,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 	request, err := d.resolve(r)
 	if err != nil {
-		if errors.Is(err, client.ErrDomainDoesNotExist) {
+		if errors.Is(err, ErrDomainDoesNotExist) {
 			// serve generic 404
 			httperrors.Serve404(w)
 			return
@@ -176,11 +164,6 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if request.LookupPath == nil {
-		httperrors.Serve404(w)
-		return
-	}
-
 	request.ServeNotFoundHTTP(w, r)
 }
 
@@ -194,16 +177,17 @@ func (d *Domain) serveNamespaceNotFound(w http.ResponseWriter, r *http.Request)
 
 	namespaceDomain, err := d.Resolver.Resolve(clonedReq)
 	if err != nil {
+		if errors.Is(err, ErrDomainDoesNotExist) {
+			// serve generic 404
+			httperrors.Serve404(w)
+			return
+		}
+
 		errortracking.Capture(err, errortracking.WithRequest(r))
 		httperrors.Serve503(w)
 		return
 	}
 
-	if namespaceDomain.LookupPath == nil {
-		httperrors.Serve404(w)
-		return
-	}
-
 	// for namespace domains that have no access control enabled
 	if !namespaceDomain.LookupPath.HasAccessControl {
 		namespaceDomain.ServeNotFoundHTTP(w, r)
@@ -222,7 +206,7 @@ func (d *Domain) ServeNotFoundAuthFailed(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	if d.IsNamespaceProject(r) && lookupPath.HasAccessControl {
+	if d.IsNamespaceProject(r) && !lookupPath.HasAccessControl {
 		d.ServeNotFoundHTTP(w, r)
 		return
 	}
diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go
index 2df1d992..f053001a 100644
--- a/internal/domain/domain_test.go
+++ b/internal/domain/domain_test.go
@@ -46,33 +46,31 @@ func TestIsHTTPSOnly(t *testing.T) {
 	}{
 		{
 			name: "Custom domain with HTTPS-only enabled",
-			domain: &Domain{
-				Resolver: &stubbedResolver{
+			domain: New("custom-domain", "", "",
+				&stubbedResolver{
 					project: &serving.LookupPath{
 						Path:        "group/project/public",
 						IsHTTPSOnly: true,
 					},
-				},
-			},
+				}),
 			url:      "http://custom-domain",
 			expected: true,
 		},
 		{
 			name: "Custom domain with HTTPS-only disabled",
-			domain: &Domain{
-				Resolver: &stubbedResolver{
+			domain: New("custom-domain", "", "",
+				&stubbedResolver{
 					project: &serving.LookupPath{
 						Path:        "group/project/public",
 						IsHTTPSOnly: false,
 					},
-				},
-			},
+				}),
 			url:      "http://custom-domain",
 			expected: false,
 		},
 		{
 			name:     "Unknown project",
-			domain:   &Domain{},
+			domain:   New("", "", "", &stubbedResolver{err: ErrDomainDoesNotExist}),
 			url:      "http://test-domain/project",
 			expected: false,
 		},
@@ -90,7 +88,7 @@ func TestPredefined404ServeHTTP(t *testing.T) {
 	cleanup := setUpTests(t)
 	defer cleanup()
 
-	testDomain := &Domain{}
+	testDomain := New("", "", "", &stubbedResolver{err: ErrDomainDoesNotExist})
 
 	testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found")
 }
@@ -192,7 +190,7 @@ func TestServeNamespaceNotFound(t *testing.T) {
 			domain: "group.404.gitlab-example.com",
 			path:   "/unknown",
 			resolver: &stubbedResolver{
-				project: nil,
+				err:     ErrDomainDoesNotExist,
 				subpath: "/",
 			},
 			expectedResponse: "The page you're looking for could not be found.",