Code review feedback

cmd/drone-server/inject_server.go

@@ -18,6 +18,7 @@ import (
 	"net/http"
 
 	"github.com/drone/drone/cmd/drone-server/config"
+	"github.com/drone/drone/core"
 	"github.com/drone/drone/handler/api"
 	"github.com/drone/drone/handler/web"
 	"github.com/drone/drone/metric"
@@ -33,9 +34,9 @@ import (
 // wire set for loading the server.
 var serverSet = wire.NewSet(
 	manager.New,
-	metric.NewServer,
 	api.New,
 	web.New,
+	provideMetric,
 	provideRouter,
 	provideRPC,
 	provideServer,
@@ -53,6 +54,12 @@ func provideRouter(api api.Server, web web.Server, rpc http.Handler, metrics *me
 	return r
 }
 
+// provideMetric is a Wire provider function that returns the
+// metrics server exposing metrics in prometheus format.
+func provideMetric(session core.Session, config config.Config) *metric.Server {
+	return metric.NewServer(session, config.Prometheus.EnableAnonymousAccess)
+}
+
 // provideRPC is a Wire provider function that returns an rpc
 // handler that exposes the build manager to a remote agent.
 func provideRPC(m manager.BuildManager, config config.Config) http.Handler {

cmd/drone-server/wire_gen.go

@@ -10,7 +10,6 @@ import (
 	"github.com/drone/drone/handler/api"
 	"github.com/drone/drone/handler/web"
 	"github.com/drone/drone/livelog"
-	"github.com/drone/drone/metric"
 	"github.com/drone/drone/operator/manager"
 	"github.com/drone/drone/pubsub"
 	"github.com/drone/drone/service/commit"
@@ -93,7 +92,7 @@ func InitializeApplication(config2 config.Config) (application, error) {
 	options := provideServerOptions(config2)
 	webServer := web.New(admissionService, buildStore, client, hookParser, coreLicense, licenseService, middleware, repositoryStore, session, syncer, triggerer, userStore, userService, webhookSender, options, system)
 	handler := provideRPC(buildManager, config2)
-	metricServer := metric.NewServer(session, config2)
+	metricServer := provideMetrics(session, config2)
 	mux := provideRouter(server, webServer, handler, metricServer)
 	serverServer := provideServer(mux, config2)
 	mainApplication := newApplication(cronScheduler, datadog, runner, serverServer, userStore)

metric/handler.go

@@ -10,7 +10,6 @@ import (
 	"errors"
 	"net/http"
 
-	"github.com/drone/drone/cmd/drone-server/config"
 	"github.com/drone/drone/core"
 
 	"github.com/prometheus/client_golang/prometheus/promhttp"
@@ -25,17 +24,17 @@ var errAccessDenied = errors.New("Access denied")
 
 // Server is an http Metrics server.
 type Server struct {
-	metrics http.Handler
+	metrics   http.Handler
-	session core.Session
+	session   core.Session
-	config  config.Config
+	anonymous bool
 }
 
 // NewServer returns a new metrics server.
-func NewServer(session core.Session, config config.Config) *Server {
+func NewServer(session core.Session, anonymous bool) *Server {
 	return &Server{
-		metrics: promhttp.Handler(),
+		metrics:   promhttp.Handler(),
-		session: session,
+		session:   session,
-		config:  config,
+		anonymous: anonymous,
 	}
 }
 
@@ -44,9 +43,9 @@ func NewServer(session core.Session, config config.Config) *Server {
 func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	user, _ := s.session.Get(r)
 	switch {
-	case !s.config.Prometheus.EnableAnonymousAccess && user == nil:
+	case !s.anonymous && user == nil:
 		http.Error(w, errInvalidToken.Error(), 401)
-	case !s.config.Prometheus.EnableAnonymousAccess && !user.Admin && !user.Machine:
+	case !s.anonymous && !user.Admin && !user.Machine:
 		http.Error(w, errAccessDenied.Error(), 403)
 	default:
 		s.metrics.ServeHTTP(w, r)

metric/handler_test.go

@@ -26,7 +26,7 @@ func TestHandleMetrics(t *testing.T) {
 	session := mock.NewMockSession(controller)
 	session.EXPECT().Get(r).Return(mockUser, nil)
 
-	NewServer(session).ServeHTTP(w, r)
+	NewServer(session, false).ServeHTTP(w, r)
 	if got, want := w.Code, 200; got != want {
 		t.Errorf("Want status code %d, got %d", want, got)
 	}
@@ -46,13 +46,30 @@ func TestHandleMetrics_NoSession(t *testing.T) {
 	session := mock.NewMockSession(controller)
 	session.EXPECT().Get(r).Return(nil, nil)
 
-	NewServer(session).ServeHTTP(w, r)
+	NewServer(session, false).ServeHTTP(w, r)
 
 	if got, want := w.Code, 401; got != want {
 		t.Errorf("Want status code %d, got %d", want, got)
 	}
 }
 
+func TestHandleMetrics_NoSessionButAnonymousAccessEnabled(t *testing.T) {
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	w := httptest.NewRecorder()
+	r := httptest.NewRequest("GET", "/", nil)
+
+	session := mock.NewMockSession(controller)
+	session.EXPECT().Get(r).Return(nil, nil)
+
+	NewServer(session, true).ServeHTTP(w, r)
+
+	if got, want := w.Code, 200; got != want {
+		t.Errorf("Want status code %d, got %d", want, got)
+	}
+}
+
 func TestHandleMetrics_AccessDenied(t *testing.T) {
 	controller := gomock.NewController(t)
 	defer controller.Finish()
@@ -64,7 +81,7 @@ func TestHandleMetrics_AccessDenied(t *testing.T) {
 	session := mock.NewMockSession(controller)
 	session.EXPECT().Get(r).Return(mockUser, nil)
 
-	NewServer(session).ServeHTTP(w, r)
+	NewServer(session, false).ServeHTTP(w, r)
 	if got, want := w.Code, 403; got != want {
 		t.Errorf("Want status code %d, got %d", want, got)
 	}