From 5ffd39e41866614de47ff427d3156062a6879e1c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sun, 24 Nov 2019 11:29:53 +0100 Subject: Respond with 502 if a domain can not be retrieved from a source --- app.go | 22 ++++++++++++++++------ internal/auth/auth.go | 11 +++++++---- internal/domain/domain.go | 6 ++---- internal/source/disk/disk.go | 17 ++++------------- internal/source/domains.go | 8 +------- internal/source/domains_test.go | 23 +++++++++-------------- internal/source/gitlab/gitlab.go | 12 +++--------- internal/source/source.go | 3 +-- 8 files changed, 43 insertions(+), 59 deletions(-) diff --git a/app.go b/app.go index 7c927dfe..f64421a4 100644 --- a/app.go +++ b/app.go @@ -61,7 +61,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.domain(ch.ServerName); domain != nil { tls, _ := domain.EnsureCertificate() return tls, nil } @@ -86,16 +86,18 @@ func (a *theApp) redirectToHTTPS(w http.ResponseWriter, r *http.Request, statusC http.Redirect(w, r, u.String(), statusCode) } -func (a *theApp) getHostAndDomain(r *http.Request) (host string, domain *domain.Domain) { +func (a *theApp) getHostAndDomain(r *http.Request) (string, *domain.Domain, error) { host, _, err := net.SplitHostPort(r.Host) if err != nil { host = r.Host } - return host, a.domain(host) + domain, err := a.domain(host) + + return host, domain, err } -func (a *theApp) domain(host string) *domain.Domain { +func (a *theApp) domain(host string) (*domain.Domain, error) { return a.domains.GetDomain(host) } @@ -163,7 +165,15 @@ func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, ht // downstream middlewares to use func (a *theApp) routingMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - host, domain := a.getHostAndDomain(r) + // if we could not retrieve a domain from domains source we break the + // middleware chain and simply respond with 502 after logging this + host, domain, err := a.getHostAndDomain(r) + if err != nil { + log.WithError(err).Error("could not fetch domain information from a source") + + httperrors.Serve502(w) + return + } r = request.WithHostAndDomain(r, host, domain) @@ -289,7 +299,7 @@ func (a *theApp) proxyInitialMiddleware(handler http.Handler) http.Handler { } func (a *theApp) buildHandlerPipeline() (http.Handler, error) { - // Handlers should be applied in reverse order + // Handlers should be applied in a reverse order handler := a.serveFileOrNotFoundHandler() if !a.DisableCrossOriginRequests { handler = corsHandler.Handler(handler) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 320dcb11..d84fc690 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -198,14 +198,17 @@ func (a *Auth) checkAuthenticationResponse(session *sessions.Session, w http.Res http.Redirect(w, r, redirectURI, 302) } -func (a *Auth) domainAllowed(domain string, domains *source.Domains) bool { - domainConfigured := (domain == a.pagesDomain) || strings.HasSuffix("."+domain, a.pagesDomain) +func (a *Auth) domainAllowed(name string, domains *source.Domains) bool { + isConfigured := (name == a.pagesDomain) || strings.HasSuffix("."+name, a.pagesDomain) - if domainConfigured { + if isConfigured { return true } - return domains.HasDomain(domain) + domain, err := domains.GetDomain(name) + + // domain exists and there is no error + return (domain != nil && err == nil) } func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, domains *source.Domains) bool { diff --git a/internal/domain/domain.go b/internal/domain/domain.go index f7eba5ca..28eb3196 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -162,10 +162,8 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { // ServeFileHTTP returns true if something was served, false if not. func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { if d.isUnconfigured() || !d.HasLookupPath(r) { - // TODO: this seems to be wrong: - // as we should rather return false, - // and fallback to `ServeNotFoundHTTP` - // to handle this case + // TODO: this seems to be wrong: as we should rather return false, and + // fallback to `ServeNotFoundHTTP` to handle this case httperrors.Serve404(w) return true } diff --git a/internal/source/disk/disk.go b/internal/source/disk/disk.go index a3ae8901..b79d222d 100644 --- a/internal/source/disk/disk.go +++ b/internal/source/disk/disk.go @@ -24,25 +24,16 @@ func New() *Disk { } } -// GetDomain returns a domain from the domains map -func (d *Disk) GetDomain(host string) *domain.Domain { +// GetDomain returns a domain from the domains map if it exists +func (d *Disk) GetDomain(host string) (*domain.Domain, error) { 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 *Disk) HasDomain(host string) bool { d.lock.RLock() defer d.lock.RUnlock() - host = strings.ToLower(host) - _, isPresent := d.dm[host] + domain, _ := d.dm[host] - return isPresent + return domain, nil } // IsReady checks if the domains source is ready for work. The disk source is diff --git a/internal/source/domains.go b/internal/source/domains.go index 92bab663..f9dadef8 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -34,16 +34,10 @@ func NewDomains() *Domains { // sources here because it allows us to switch behavior and the domain source // for some subset of domains, to test / PoC the new GitLab Domains Source that // we plan to use to replace the disk source. -func (d *Domains) GetDomain(name string) *domain.Domain { +func (d *Domains) GetDomain(name string) (*domain.Domain, error) { return d.source(name).GetDomain(name) } -// HasDomain checks if domain exists. It is using new and the legacy domains -// source. -func (d *Domains) HasDomain(name string) bool { - return d.source(name).HasDomain(name) -} - // Start starts the disk domain source. It is DEPRECATED, because we want to // remove it entirely when disk source gets removed. func (d *Domains) Read(rootDomain string) { diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 1da50db8..64ec5f8d 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" @@ -14,27 +15,20 @@ type mockSource struct { mock.Mock } -func (m mockSource) GetDomain(name string) *domain.Domain { +func (m mockSource) GetDomain(name string) (*domain.Domain, error) { args := m.Called(name) - return args.Get(0).(*domain.Domain) + return args.Get(0).(*domain.Domain), args.Error(1) } -func (m mockSource) HasDomain(name string) bool { - args := m.Called(name) - - return args.Bool(0) -} - -func TestSourceTransition(t *testing.T) { +func TestHasDomain(t *testing.T) { testDomain := newSourceDomains[0] t.Run("when requesting a test domain", func(t *testing.T) { newSource := new(mockSource) newSource.On("GetDomain", testDomain). - Return(&domain.Domain{Name: testDomain}). + Return(&domain.Domain{Name: testDomain}, nil). Once() - newSource.On("HasDomain", testDomain).Return(true).Once() defer newSource.AssertExpectations(t) domains := &Domains{ @@ -43,7 +37,6 @@ func TestSourceTransition(t *testing.T) { } domains.GetDomain(testDomain) - domains.HasDomain(testDomain) }) t.Run("when requesting a non-test domain", func(t *testing.T) { @@ -55,7 +48,9 @@ func TestSourceTransition(t *testing.T) { gitlab: newSource, } - assert.Nil(t, domains.GetDomain("some.test.io")) - assert.False(t, domains.HasDomain("some.test.io")) + domain, err := domains.GetDomain("domain.test.io") + + require.NoError(t, err) + assert.Nil(t, domain) }) } diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index b0efb01d..09cd06b9 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -1,6 +1,7 @@ package gitlab import ( + "errors" "net/http" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" @@ -21,15 +22,8 @@ func New() *Gitlab { // GetDomain return a representation of a domain that we have fetched from // GitLab -// It should return source.Lookup TODO -func (g *Gitlab) GetDomain(name string) *domain.Domain { - return nil -} - -// HasDomain checks if a domain is known to GitLab -// TODO lookup status code etc. -func (g *Gitlab) HasDomain(name string) bool { - return g.GetDomain(name) != nil +func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) { + return nil, errors.New("not implemented") } // Resolve is supposed to get the serving lookup path based on the request from diff --git a/internal/source/source.go b/internal/source/source.go index da63cbcc..4b43b8f4 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -4,6 +4,5 @@ import "gitlab.com/gitlab-org/gitlab-pages/internal/domain" // Source represents an abstract interface of a domains configuration source. type Source interface { - GetDomain(string) *domain.Domain - HasDomain(string) bool + GetDomain(string) (*domain.Domain, error) } -- cgit v1.2.3