WIP670-pipeline-refactor-1

8 files changed, 75 insertions, 52 deletions

diff --git a/app.go b/app.go
index c9d61c40..381cd8e3 100644
--- a/app.go
+++ b/app.go
@@ -71,36 +71,14 @@ func (a *theApp) ServeTLS(ch *cryptotls.ClientHelloInfo) (*cryptotls.Certificate
 	return nil, nil
 }
 
-func redirectToHTTPS(w http.ResponseWriter, r *http.Request, statusCode int) {
-	u := *r.URL
-	u.Scheme = request.SchemeHTTPS
-	u.Host = r.Host
-	u.User = nil
-
-	http.Redirect(w, r, u.String(), statusCode)
-}
-
 func (a *theApp) domain(ctx context.Context, host string) (*domain.Domain, error) {
 	return a.source.GetDomain(ctx, host)
 }
 
-// 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) {
-	// To avoid user knowing if pages exist, we will force user to login and authorize pages
-	if a.Auth.CheckAuthenticationWithoutProject(w, r, domain) {
-		return
-	}
-
-	// auth succeeded try to serve the correct 404 page
-	domain.ServeNotFoundAuthFailed(w, r)
-}
-
 func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, https bool, host string, domain *domain.Domain) bool {
 	// Add auto redirect
 	if !https && a.config.General.RedirectHTTP {
-		redirectToHTTPS(w, r, http.StatusTemporaryRedirect)
+		handlers.RedirectToHTTPS(w, r, http.StatusTemporaryRedirect)
 		return true
 	}
 
@@ -121,12 +99,12 @@ func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, ht
 		}
 
 		// redirect to auth and serve not found
-		a.checkAuthAndServeNotFound(domain, w, r)
+		handlers.CheckAuthAndServeNotFound(a.Auth, domain, w, r)
 		return true
 	}
 
 	if !https && domain.IsHTTPSOnly(r) {
-		redirectToHTTPS(w, r, http.StatusMovedPermanently)
+		handlers.RedirectToHTTPS(w, r, http.StatusMovedPermanently)
 		return true
 	}
 
@@ -209,7 +187,7 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) {
 	// Handlers should be applied in a reverse order
 	handler := handlers.ServeFileOrNotFound(a.Auth)
 	handler = handlers.Cors(a.config, handler)
-	handler = handlers.Authorization(a.Auth, handler)
+	handler = a.Handlers.Authorization(handler)
 	handler = a.auxiliaryMiddleware(handler)
 	handler = handlers.Authentication(a.Auth, a.source, handler)
 	handler = a.AcmeMiddleware.AcmeMiddleware(handler)
@@ -399,7 +377,7 @@ func runApp(config *cfg.Config) {
 
 	a.setAuth(config)
 
-	a.Handlers = handlers.New(a.Auth, a.Artifact)
+	a.Handlers = handlers.New(a.config, a.Auth, a.Artifact)
 
 	// TODO: This if was introduced when `gitlab-server` wasn't a required parameter
 	// once we completely remove support for legacy architecture and make it required
diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go
index 815f4755..243f62ec 100644
--- a/internal/handlers/auth.go
+++ b/internal/handlers/auth.go
@@ -4,13 +4,27 @@ import (
 	"net/http"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/auth"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source"
 )
 
-func Authorization(auth *auth.Auth, handler http.Handler) http.Handler {
-	return auth.AuthorizationMiddleware(handler)
+func (h *Handlers) Authorization(handler http.Handler) http.Handler {
+	return h.Auth.AuthorizationMiddleware(handler)
 }
 
 func Authentication(auth *auth.Auth, s source.Source, handler http.Handler) http.Handler {
 	return auth.AuthenticationMiddleware(handler, s)
 }
+
+// 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 CheckAuthAndServeNotFound(a *auth.Auth, domain *domain.Domain, w http.ResponseWriter, r *http.Request) {
+	// To avoid user knowing if pages exist, we will force user to login and authorize pages
+	if a.CheckAuthenticationWithoutProject(w, r, domain) {
+		return
+	}
+
+	// auth succeeded try to serve the correct 404 page
+	domain.ServeNotFoundAuthFailed(w, r)
+}
diff --git a/internal/handlers/auxiliary.go b/internal/handlers/auxiliary.go
new file mode 100644
index 00000000..443e6416
--- /dev/null
+++ b/internal/handlers/auxiliary.go
@@ -0,0 +1,16 @@
+package handlers
+
+import (
+	"net/http"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
+)
+
+func RedirectToHTTPS(w http.ResponseWriter, r *http.Request, statusCode int) {
+	u := *r.URL
+	u.Scheme = request.SchemeHTTPS
+	u.Host = r.Host
+	u.User = nil
+
+	http.Redirect(w, r, u.String(), statusCode)
+}
diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go
index 76724387..a3988c58 100644
--- a/internal/handlers/handlers.go
+++ b/internal/handlers/handlers.go
@@ -4,30 +4,33 @@ import (
 	"net/http"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/logging"
 )
 
 // Handlers take care of handling specific requests
 type Handlers struct {
+	config   *config.Config
 	Auth     internal.Auth
 	Artifact internal.Artifact
 }
 
 // New when provided the arguments defined herein, returns a pointer to an
 // Handlers that is used to handle requests.
-func New(auth internal.Auth, artifact internal.Artifact) *Handlers {
+func New(config *config.Config, auth internal.Auth, artifact internal.Artifact) *Handlers {
 	return &Handlers{
+		config:   config,
 		Auth:     auth,
 		Artifact: artifact,
 	}
 }
 
-func (a *Handlers) checkIfLoginRequiredOrInvalidToken(w http.ResponseWriter, r *http.Request, token string) func(*http.Response) bool {
+func (h *Handlers) checkIfLoginRequiredOrInvalidToken(w http.ResponseWriter, r *http.Request, token string) func(*http.Response) bool {
 	return func(resp *http.Response) bool {
 		// API will return 403 if the project does not have public pipelines (public_builds flag)
 		if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusForbidden {
 			if token == "" {
-				if !a.Auth.IsAuthSupported() {
+				if !h.Auth.IsAuthSupported() {
 					// Auth is not supported, probably means no access or does not exist but we cannot try with auth
 					return false
 				}
@@ -35,7 +38,7 @@ func (a *Handlers) checkIfLoginRequiredOrInvalidToken(w http.ResponseWriter, r *
 				logging.LogRequest(r).Debugf("Artifact API response was %d without token, try with authentication", resp.StatusCode)
 
 				// Authenticate user
-				if a.Auth.RequireAuth(w, r) {
+				if h.Auth.RequireAuth(w, r) {
 					return true
 				}
 			} else {
@@ -43,7 +46,7 @@ func (a *Handlers) checkIfLoginRequiredOrInvalidToken(w http.ResponseWriter, r *
 			}
 		}
 
-		if a.Auth.CheckResponseForInvalidToken(w, r, resp) {
+		if h.Auth.CheckResponseForInvalidToken(w, r, resp) {
 			return true
 		}
 
@@ -52,18 +55,18 @@ func (a *Handlers) checkIfLoginRequiredOrInvalidToken(w http.ResponseWriter, r *
 }
 
 // HandleArtifactRequest handles all artifact related requests, will return true if request was handled here
-func (a *Handlers) HandleArtifactRequest(host string, w http.ResponseWriter, r *http.Request) bool {
-	// In the event a host is prefixed with the artifact prefix an artifact
+func (h *Handlers) HandleArtifactRequest(host string, w http.ResponseWriter, r *http.Request) bool {
+	// In the event h host is prefixed with the artifact prefix an artifact
 	// value is created, and an attempt to proxy the request is made
 
 	// Always try to add token to the request if it exists
-	token, err := a.Auth.GetTokenIfExists(w, r)
+	token, err := h.Auth.GetTokenIfExists(w, r)
 	if err != nil {
 		return true
 	}
 
 	// nolint: bodyclose
-	// a.checkIfLoginRequiredOrInvalidToken returns a response.Body, closing this body is responsibility
+	// h.checkIfLoginRequiredOrInvalidToken returns h response.Body, closing this body is responsibility
 	// of the TryMakeRequest implementation
-	return a.Artifact.TryMakeRequest(host, w, r, token, a.checkIfLoginRequiredOrInvalidToken(w, r, token))
+	return h.Artifact.TryMakeRequest(host, w, r, token, h.checkIfLoginRequiredOrInvalidToken(w, r, token))
 }
diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go
index d71e9b91..f63c57c9 100644
--- a/internal/handlers/handlers_test.go
+++ b/internal/handlers/handlers_test.go
@@ -7,10 +7,11 @@ import (
 	"net/url"
 	"testing"
 
-	"gitlab.com/gitlab-org/gitlab-pages/internal/mocks"
-
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/mocks"
 )
 
 func TestNotHandleArtifactRequestReturnsFalse(t *testing.T) {
@@ -28,7 +29,7 @@ func TestNotHandleArtifactRequestReturnsFalse(t *testing.T) {
 		Return("", nil).
 		Times(1)
 
-	handlers := New(mockAuth, mockArtifact)
+	handlers := New(&config.Config{}, mockAuth, mockArtifact)
 
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/something")
@@ -53,7 +54,7 @@ func TestHandleArtifactRequestedReturnsTrue(t *testing.T) {
 		Return("", nil).
 		Times(1)
 
-	handlers := New(mockAuth, mockArtifact)
+	handlers := New(&config.Config{}, mockAuth, mockArtifact)
 
 	result := httptest.NewRecorder()
 	r := httptest.NewRequest(http.MethodGet, "/something", nil)
@@ -68,7 +69,7 @@ func TestNotFoundWithTokenIsNotHandled(t *testing.T) {
 	mockAuth.EXPECT().CheckResponseForInvalidToken(gomock.Any(), gomock.Any(), gomock.Any()).
 		Return(false)
 
-	handlers := New(mockAuth, nil)
+	handlers := New(&config.Config{}, mockAuth, nil)
 
 	w := httptest.NewRecorder()
 	reqURL, _ := url.Parse("/")
@@ -110,7 +111,7 @@ func TestForbiddenWithTokenIsNotHandled(t *testing.T) {
 					Return(false)
 			}
 
-			handlers := New(mockAuth, nil)
+			handlers := New(&config.Config{}, mockAuth, nil)
 
 			w := httptest.NewRecorder()
 			r := httptest.NewRequest(http.MethodGet, "/", nil)
@@ -128,7 +129,7 @@ func TestNotFoundWithoutTokenIsNotHandledWhenNotAuthSupport(t *testing.T) {
 	mockAuth := mocks.NewMockAuth(mockCtrl)
 	mockAuth.EXPECT().IsAuthSupported().Return(false)
 
-	handlers := New(mockAuth, nil)
+	handlers := New(&config.Config{}, mockAuth, nil)
 
 	w := httptest.NewRecorder()
 	r := httptest.NewRequest(http.MethodGet, "/", nil)
@@ -144,7 +145,7 @@ func TestNotFoundWithoutTokenIsHandled(t *testing.T) {
 	mockAuth.EXPECT().IsAuthSupported().Return(true)
 	mockAuth.EXPECT().RequireAuth(gomock.Any(), gomock.Any()).Times(1).Return(true)
 
-	handlers := New(mockAuth, nil)
+	handlers := New(&config.Config{}, mockAuth, nil)
 
 	w := httptest.NewRecorder()
 	r := httptest.NewRequest(http.MethodGet, "/", nil)
@@ -160,7 +161,7 @@ func TestInvalidTokenResponseIsHandled(t *testing.T) {
 	mockAuth.EXPECT().CheckResponseForInvalidToken(gomock.Any(), gomock.Any(), gomock.Any()).
 		Return(true)
 
-	handlers := New(mockAuth, nil)
+	handlers := New(&config.Config{}, mockAuth, nil)
 
 	w := httptest.NewRecorder()
 	r := httptest.NewRequest(http.MethodGet, "/", nil)
@@ -181,7 +182,7 @@ func TestHandleArtifactRequestButGetTokenFails(t *testing.T) {
 	mockAuth := mocks.NewMockAuth(mockCtrl)
 	mockAuth.EXPECT().GetTokenIfExists(gomock.Any(), gomock.Any()).Return("", errors.New("error when retrieving token"))
 
-	handlers := New(mockAuth, mockArtifact)
+	handlers := New(&config.Config{}, mockAuth, mockArtifact)
 
 	result := httptest.NewRecorder()
 	r := httptest.NewRequest(http.MethodGet, "/something", nil)
diff --git a/internal/handlers/servefile.go b/internal/handlers/servefile.go
index d55335e2..b94dcdcf 100644
--- a/internal/handlers/servefile.go
+++ b/internal/handlers/servefile.go
@@ -4,14 +4,13 @@ import (
 	"net/http"
 	"time"
 
-	"gitlab.com/gitlab-org/gitlab-pages/internal/auth"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
 // ServeFileOrNotFound will serve static content or
 // return a 404 Not Found response
-func ServeFileOrNotFound(auth *auth.Auth) http.Handler {
+func (h *Handlers) ServeFileOrNotFound() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		start := time.Now()
 		defer metrics.ServingTime.Observe(time.Since(start).Seconds())
@@ -24,7 +23,7 @@ func ServeFileOrNotFound(auth *auth.Auth) http.Handler {
 			// because the projects override the paths of the namespace project and they might be private even though
 			// namespace project is public
 			if d.IsNamespaceProject(r) {
-				if auth.CheckAuthenticationWithoutProject(w, r, d) {
+				if h.Auth.CheckAuthenticationWithoutProject(w, r, d) {
 					return
 				}
 			}
diff --git a/internal/interface.go b/internal/interface.go
index 3e82ee3a..63275cc1 100644
--- a/internal/interface.go
+++ b/internal/interface.go
@@ -11,8 +11,10 @@ type Artifact interface {
 
 // Auth handles the authentication logic
 type Auth interface {
+	AuthorizationMiddleware(handler http.Handler) http.Handler
 	IsAuthSupported() bool
 	RequireAuth(w http.ResponseWriter, r *http.Request) bool
 	GetTokenIfExists(w http.ResponseWriter, r *http.Request) (string, error)
 	CheckResponseForInvalidToken(w http.ResponseWriter, r *http.Request, resp *http.Response) bool
+	CheckAuthenticationWithoutProject(w http.ResponseWriter, r *http.Request, domain domain) bool
 }
diff --git a/internal/mocks/mocks.go b/internal/mocks/mocks.go
index b18bede4..7bf9562c 100644
--- a/internal/mocks/mocks.go
+++ b/internal/mocks/mocks.go
@@ -122,6 +122,16 @@ func (m *MockAuth) RequireAuth(w http.ResponseWriter, r *http.Request) bool {
 	return ret0
 }
 
+// AuthorizationMiddleware handles authorization
+func (m *MockAuth) AuthorizationMiddleware(handler http.Handler) http.Handler {
+	return handler
+}
+
+// CheckAuthenticationWithoutProject checks if user is authenticated and has a valid token
+func (a *Auth) CheckAuthenticationWithoutProject(w http.ResponseWriter, r *http.Request, domain domain) bool {
+	return false
+}
+
 // RequireAuth indicates an expected call of RequireAuth.
 func (mr *MockAuthMockRecorder) RequireAuth(w, r interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()