Skip to content

Commit

Permalink
adapt the data service's request handlers to rest.HandlerFunc
Browse files Browse the repository at this point in the history
When I started working on the alerts configs, I had a discussion with Todd
about routing, and how the data service for some unknown reason uses its own
authentication method, rather than the various api.Require* methods provided
by platform and used in the other services.

In the process of working on #671
it became clear to me that the reason was the difference in the function
signatures of request handlers in the data service as opposed to those in
other services. Specifically, data service request handlers have this
signature:

    func Foo(service.Context)

Whereas rest.HandlerFunc has this signature:

    func Foo(rest.ResponseWriter, *rest.Request)

So I modified the data service's MakeRoute function to accept a list of
rest.MiddlewareSimple, and added a ToRestRoute method that when provided with
the existing adapter function in the data api service, would convert the data
service routes to *rest.Route, while also applying middleware.

The result is that data service routes can be specified including
rest.MiddlewareSimple (like api.Require), and we no longer require a separate
Authentication helper for data service routes.

There are some TODO comments that indicate that going further down the data
service handler rabbit hole might not have been the intended path, but rather
moving to a rest.HandlerFunc signature was at one time the plan (in 2018 or
so). If so, this should only make it clearer what needs to change to effect
that migration.
  • Loading branch information
ewollesen committed Oct 18, 2023
1 parent 14c054b commit 9088da2
Show file tree
Hide file tree
Showing 15 changed files with 145 additions and 123 deletions.
4 changes: 2 additions & 2 deletions auth/service/api/v1/restricted_token.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ import (
func (r *Router) RestrictedTokensRoutes() []*rest.Route {
return []*rest.Route{
rest.Get("/v1/users/:userId/restricted_tokens", api.RequireServer(r.ListUserRestrictedTokens)),
rest.Post("/v1/users/:userId/restricted_tokens", api.Require(r.CreateUserRestrictedToken)),
rest.Post("/v1/users/:userId/restricted_tokens", api.RequireAuth(r.CreateUserRestrictedToken)),
rest.Delete("/v1/users/:userId/restricted_tokens", api.RequireServer(r.DeleteAllRestrictedTokens)),
rest.Get("/v1/restricted_tokens/:id", api.RequireServer(r.GetRestrictedToken)),
rest.Put("/v1/restricted_tokens/:id", api.RequireServer(r.UpdateRestrictedToken)),
rest.Delete("/v1/restricted_tokens/:id", api.Require(r.DeleteRestrictedToken)),
rest.Delete("/v1/restricted_tokens/:id", api.RequireAuth(r.DeleteRestrictedToken)),
}
}

Expand Down
10 changes: 3 additions & 7 deletions data/service/api/standard.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,19 +72,15 @@ func NewStandard(svc service.Service, metricClient metric.Client, permissionClie

func (s *Standard) DEPRECATEDInitializeRouter(routes []dataService.Route) error {
baseRoutes := []dataService.Route{
dataService.MakeRoute("GET", "/status", s.StatusGet),
dataService.MakeRoute("GET", "/version", s.VersionGet),
dataService.Get("/status", s.StatusGet),
dataService.Get("/version", s.VersionGet),
}

routes = append(baseRoutes, routes...)

var contextRoutes []*rest.Route
for _, route := range routes {
contextRoutes = append(contextRoutes, &rest.Route{
HttpMethod: route.Method,
PathExp: route.Path,
Func: s.withContext(route.Handler),
})
contextRoutes = append(contextRoutes, route.ToRestRoute(s.withContext))
}

router, err := rest.MakeRouter(contextRoutes...)
Expand Down
7 changes: 4 additions & 3 deletions data/service/api/v1/alerts.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import (
"github.com/tidepool-org/platform/permission"
"github.com/tidepool-org/platform/request"
platform "github.com/tidepool-org/platform/service"
"github.com/tidepool-org/platform/service/api"
)

func AlertsRoutes() []service.Route {
return []service.Route{
service.MakeRoute("GET", "/v1/alerts/:userID/:followedUserID", EnforceAuthentication(GetAlert)),
service.MakeRoute("POST", "/v1/alerts/:userID/:followedUserID", EnforceAuthentication(UpsertAlert)),
service.MakeRoute("DELETE", "/v1/alerts/:userID/:followedUserID", EnforceAuthentication(DeleteAlert)),
service.Get("/v1/alerts/:userID/:followedUserID", GetAlert, api.RequireAuth),
service.Post("/v1/alerts/:userID/:followedUserID", UpsertAlert, api.RequireAuth),
service.Delete("/v1/alerts/:userID/:followedUserID", DeleteAlert, api.RequireAuth),
}
}

Expand Down
33 changes: 0 additions & 33 deletions data/service/api/v1/authenticate_middleware.go

This file was deleted.

13 changes: 3 additions & 10 deletions data/service/api/v1/data_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,13 @@ import (
"github.com/tidepool-org/platform/page"
"github.com/tidepool-org/platform/permission"
"github.com/tidepool-org/platform/request"
"github.com/tidepool-org/platform/service/api"
)

// TODO: BEGIN: Update to new service paradigm
// func (r *Router) DataSetsRoutes() []*rest.Route {
// return []*rest.Route{
// rest.Get("/v1/users/:userId/data_sets", api.Require(r.ListUserDataSets)),
// rest.Get("/v1/data_sets/:id", api.Require(r.GetDataSet)),
// }
// }

func DataSetsRoutes() []dataService.Route {
return []dataService.Route{
dataService.MakeRoute("GET", "/v1/users/:userId/data_sets", EnforceAuthentication(ListUserDataSets)),
dataService.MakeRoute("GET", "/v1/data_sets/:dataSetId", EnforceAuthentication(GetDataSet)),
dataService.Get("/v1/users/:userId/data_sets", ListUserDataSets, api.RequireAuth),
dataService.Get("/v1/data_sets/:dataSetId", GetDataSet, api.RequireAuth),
}
}

Expand Down
24 changes: 7 additions & 17 deletions data/service/api/v1/data_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,17 @@ import (
dataSource "github.com/tidepool-org/platform/data/source"
"github.com/tidepool-org/platform/page"
"github.com/tidepool-org/platform/request"
"github.com/tidepool-org/platform/service/api"
)

// TODO: BEGIN: Update to new service paradigm
// func (r *Router) SourcesRoutes() []*rest.Route {
// return []*rest.Route{
// rest.Get("/v1/users/:userId/data_sources", api.Require(r.ListSources)),
// rest.Post("/v1/users/:userId/data_sources", api.RequireServer(r.CreateSource)),
// rest.Get("/v1/data_sources/:id", api.Require(r.GetSource)),
// rest.Put("/v1/data_sources/:id", api.RequireServer(r.UpdateSource)),
// rest.Delete("/v1/data_sources/:id", api.RequireServer(r.DeleteSource)),
// }
// }

func SourcesRoutes() []dataService.Route {
return []dataService.Route{
dataService.MakeRoute("GET", "/v1/users/:userId/data_sources", EnforceAuthentication(ListSources)),
dataService.MakeRoute("POST", "/v1/users/:userId/data_sources", EnforceAuthentication(CreateSource)),
dataService.MakeRoute("DELETE", "/v1/users/:userId/data_sources", EnforceAuthentication(DeleteAllSources)),
dataService.MakeRoute("GET", "/v1/data_sources/:id", EnforceAuthentication(GetSource)),
dataService.MakeRoute("PUT", "/v1/data_sources/:id", EnforceAuthentication(UpdateSource)),
dataService.MakeRoute("DELETE", "/v1/data_sources/:id", EnforceAuthentication(DeleteSource)),
dataService.Get("/v1/users/:userId/data_sources", ListSources, api.RequireAuth),
dataService.Post("/v1/users/:userId/data_sources", CreateSource, api.RequireAuth),
dataService.Delete("/v1/users/:userId/data_sources", DeleteAllSources, api.RequireAuth),
dataService.Get("/v1/data_sources/:id", GetSource, api.RequireAuth),
dataService.Put("/v1/data_sources/:id", UpdateSource, api.RequireAuth),
dataService.Delete("/v1/data_sources/:id", DeleteSource, api.RequireAuth),
}
}

Expand Down
21 changes: 11 additions & 10 deletions data/service/api/v1/summary.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
dataService "github.com/tidepool-org/platform/data/service"
"github.com/tidepool-org/platform/data/summary"
"github.com/tidepool-org/platform/data/summary/types"
"github.com/tidepool-org/platform/service/api"

"github.com/tidepool-org/platform/page"
"github.com/tidepool-org/platform/permission"
Expand All @@ -16,20 +17,20 @@ import (

func SummaryRoutes() []dataService.Route {
return []dataService.Route{
dataService.MakeRoute("GET", "/v1/summaries/cgm/:userId", EnforceAuthentication(GetSummary[types.CGMStats, *types.CGMStats])),
dataService.MakeRoute("GET", "/v1/summaries/bgm/:userId", EnforceAuthentication(GetSummary[types.BGMStats, *types.BGMStats])),
dataService.Get("/v1/summaries/cgm/:userId", GetSummary[types.CGMStats, *types.CGMStats], api.RequireAuth),
dataService.Get("/v1/summaries/bgm/:userId", GetSummary[types.BGMStats, *types.BGMStats], api.RequireAuth),

dataService.MakeRoute("POST", "/v1/summaries/cgm/:userId", EnforceAuthentication(UpdateSummary[types.CGMStats, *types.CGMStats])),
dataService.MakeRoute("POST", "/v1/summaries/bgm/:userId", EnforceAuthentication(UpdateSummary[types.BGMStats, *types.BGMStats])),
dataService.Post("/v1/summaries/cgm/:userId", UpdateSummary[types.CGMStats, *types.CGMStats], api.RequireAuth),
dataService.Post("/v1/summaries/bgm/:userId", UpdateSummary[types.BGMStats, *types.BGMStats], api.RequireAuth),

dataService.MakeRoute("POST", "/v1/summaries/backfill/cgm", EnforceAuthentication(BackfillSummaries[types.CGMStats, *types.CGMStats])),
dataService.MakeRoute("POST", "/v1/summaries/backfill/bgm", EnforceAuthentication(BackfillSummaries[types.BGMStats, *types.BGMStats])),
dataService.Post("/v1/summaries/backfill/cgm", BackfillSummaries[types.CGMStats, *types.CGMStats], api.RequireAuth),
dataService.Post("/v1/summaries/backfill/bgm", BackfillSummaries[types.BGMStats, *types.BGMStats], api.RequireAuth),

dataService.MakeRoute("GET", "/v1/summaries/outdated/cgm", EnforceAuthentication(GetOutdatedUserIDs[types.CGMStats, *types.CGMStats])),
dataService.MakeRoute("GET", "/v1/summaries/outdated/bgm", EnforceAuthentication(GetOutdatedUserIDs[types.BGMStats, *types.BGMStats])),
dataService.Get("/v1/summaries/outdated/cgm", GetOutdatedUserIDs[types.CGMStats, *types.CGMStats], api.RequireAuth),
dataService.Get("/v1/summaries/outdated/bgm", GetOutdatedUserIDs[types.BGMStats, *types.BGMStats], api.RequireAuth),

dataService.MakeRoute("GET", "/v1/summaries/migratable/cgm", EnforceAuthentication(GetMigratableUserIDs[types.CGMStats, *types.CGMStats])),
dataService.MakeRoute("GET", "/v1/summaries/migratable/bgm", EnforceAuthentication(GetMigratableUserIDs[types.BGMStats, *types.BGMStats])),
dataService.Get("/v1/summaries/migratable/cgm", GetMigratableUserIDs[types.CGMStats, *types.CGMStats], api.RequireAuth),
dataService.Get("/v1/summaries/migratable/bgm", GetMigratableUserIDs[types.BGMStats, *types.BGMStats], api.RequireAuth),
}
}

Expand Down
29 changes: 16 additions & 13 deletions data/service/api/v1/v1.go
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
package v1

import "github.com/tidepool-org/platform/data/service"
import (
"github.com/tidepool-org/platform/data/service"
"github.com/tidepool-org/platform/service/api"
)

func Routes() []service.Route {
routes := []service.Route{
service.MakeRoute("POST", "/v1/datasets/:dataSetId/data", EnforceAuthentication(DataSetsDataCreate)),
service.MakeRoute("DELETE", "/v1/datasets/:dataSetId", EnforceAuthentication(DataSetsDelete)),
service.MakeRoute("PUT", "/v1/datasets/:dataSetId", EnforceAuthentication(DataSetsUpdate)),
service.MakeRoute("DELETE", "/v1/users/:userId/data", EnforceAuthentication(UsersDataDelete)),
service.MakeRoute("POST", "/v1/users/:userId/datasets", EnforceAuthentication(UsersDataSetsCreate)),
service.MakeRoute("GET", "/v1/users/:userId/datasets", EnforceAuthentication(UsersDataSetsGet)),
service.Post("/v1/datasets/:dataSetId/data", DataSetsDataCreate, api.RequireAuth),
service.Delete("/v1/datasets/:dataSetId", DataSetsDelete, api.RequireAuth),
service.Put("/v1/datasets/:dataSetId", DataSetsUpdate, api.RequireAuth),
service.Delete("/v1/users/:userId/data", UsersDataDelete, api.RequireAuth),
service.Post("/v1/users/:userId/datasets", UsersDataSetsCreate, api.RequireAuth),
service.Get("/v1/users/:userId/datasets", UsersDataSetsGet, api.RequireAuth),

service.MakeRoute("POST", "/v1/data_sets/:dataSetId/data", EnforceAuthentication(DataSetsDataCreate)),
service.MakeRoute("DELETE", "/v1/data_sets/:dataSetId/data", EnforceAuthentication(DataSetsDataDelete)),
service.MakeRoute("DELETE", "/v1/data_sets/:dataSetId", EnforceAuthentication(DataSetsDelete)),
service.MakeRoute("PUT", "/v1/data_sets/:dataSetId", EnforceAuthentication(DataSetsUpdate)),
service.MakeRoute("GET", "/v1/time", TimeGet),
service.MakeRoute("POST", "/v1/users/:userId/data_sets", EnforceAuthentication(UsersDataSetsCreate)),
service.Post("/v1/data_sets/:dataSetId/data", DataSetsDataCreate, api.RequireAuth),
service.Delete("/v1/data_sets/:dataSetId/data", DataSetsDataDelete, api.RequireAuth),
service.Delete("/v1/data_sets/:dataSetId", DataSetsDelete, api.RequireAuth),
service.Put("/v1/data_sets/:dataSetId", DataSetsUpdate, api.RequireAuth),
service.Get("/v1/time", TimeGet),
service.Post("/v1/users/:userId/data_sets", UsersDataSetsCreate, api.RequireAuth),
}

routes = append(routes, DataSetsRoutes()...)
Expand Down
66 changes: 59 additions & 7 deletions data/service/route.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,67 @@
package service

import (
"net/http"

"github.com/ant0ine/go-json-rest/rest"
)

type Route struct {
Method string
Path string
Handler HandlerFunc
Handler HandlerFunc
Method string
Path string
middleware []rest.MiddlewareSimple
}

func MakeRoute(method string, path string, handler HandlerFunc) Route {
// MakeRoute builds a Route.
//
// Consider using the handy Get, Post, etc helpers.
func MakeRoute(method string, path string, handler HandlerFunc, middleware ...rest.MiddlewareSimple) Route {
return Route{
Method: method,
Path: path,
Handler: handler,
Method: method,
Path: path,
Handler: handler,
middleware: middleware,
}
}

// Delete wraps MakeRoute for easy DELETE route creation.
func Delete(path string, handler HandlerFunc, middleware ...rest.MiddlewareSimple) Route {
return MakeRoute(http.MethodDelete, path, handler, middleware...)
}

// Get wraps MakeRoute for easy GET route creation.
func Get(path string, handler HandlerFunc, middleware ...rest.MiddlewareSimple) Route {
return MakeRoute(http.MethodGet, path, handler, middleware...)
}

// Patch wraps MakeRoute for easy PATCH route creation.
func Patch(path string, handler HandlerFunc, middleware ...rest.MiddlewareSimple) Route {
return MakeRoute(http.MethodPatch, path, handler, middleware...)
}

// Post wraps MakeRoute for easy POST route creation.
func Post(path string, handler HandlerFunc, middleware ...rest.MiddlewareSimple) Route {
return MakeRoute(http.MethodPost, path, handler, middleware...)
}

// Put wraps MakeRoute for easy PUT route creation.
func Put(path string, handler HandlerFunc, middleware ...rest.MiddlewareSimple) Route {
return MakeRoute(http.MethodPut, path, handler, middleware...)
}

// RestRouteAdapterFunc adapts a HandlerFunc to a rest.HandlerFunc.
type RestRouteAdapterFunc (func(HandlerFunc) rest.HandlerFunc)

// ToRestRoute converts a Route to a rest.Route.
func (r *Route) ToRestRoute(f RestRouteAdapterFunc) *rest.Route {
var middlewares []rest.Middleware
for _, s := range r.middleware {
middlewares = append(middlewares, rest.MiddlewareSimple(s))
}
return &rest.Route{
HttpMethod: r.Method,
PathExp: r.Path,
Func: rest.WrapMiddlewares(middlewares, f(r.Handler)),
}
}
4 changes: 2 additions & 2 deletions prescription/api/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ func (r *Router) Routes() []*rest.Route {
rest.Delete("/v1/clinics/:clinicId/prescriptions/:prescriptionId", api.RequireUser(r.DeletePrescription)),

rest.Post("/v1/patients/:userId/prescriptions", api.RequireUser(r.ClaimPrescription)),
rest.Get("/v1/patients/:userId/prescriptions", api.Require(r.ListUserPrescriptions)),
rest.Get("/v1/patients/:userId/prescriptions/:prescriptionId", api.Require(r.GetPatientPrescription)),
rest.Get("/v1/patients/:userId/prescriptions", api.RequireAuth(r.ListUserPrescriptions)),
rest.Get("/v1/patients/:userId/prescriptions/:prescriptionId", api.RequireAuth(r.GetPatientPrescription)),
rest.Patch("/v1/patients/:userId/prescriptions/:prescriptionId", api.RequireUser(r.UpdateState)),
}
}
2 changes: 1 addition & 1 deletion service/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (a *API) InitializeMiddleware() error {
if err != nil {
return err
}
authMiddleware, err := middleware.NewAuth(a.Secret(), a.AuthClient())
authMiddleware, err := middleware.NewAuthenticator(a.Secret(), a.AuthClient())
if err != nil {
return err
}
Expand Down
18 changes: 17 additions & 1 deletion service/api/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,21 @@ import (
"github.com/tidepool-org/platform/request"
)

func Require(handlerFunc rest.HandlerFunc) rest.HandlerFunc {
// RequireAuth aborts with an error if a request isn't authenticated.
//
// Requests with incorrect, invalid, or no credentials are rejected.
//
// RequireAuth works by checking for the existence of an AuthDetails sentinel
// value, which implies an important assumption:
//
// The existence of AuthDetails in the request's Context indicates that the
// request has already been properly authenticated.
//
// The function that performs the actual authentication is in the
// service/middleware package. As long as no other code adds an AuthDetails
// value to the request's Context (when the request isn't properly
// authenticated) then things should be good.
func RequireAuth(handlerFunc rest.HandlerFunc) rest.HandlerFunc {
return func(res rest.ResponseWriter, req *rest.Request) {
if handlerFunc != nil && res != nil && req != nil {
if details := request.GetAuthDetails(req.Context()); details == nil {
Expand All @@ -20,6 +34,7 @@ func Require(handlerFunc rest.HandlerFunc) rest.HandlerFunc {
}
}

// RequireServer aborts with an error if a request isn't authenticated as a server.
func RequireServer(handlerFunc rest.HandlerFunc) rest.HandlerFunc {
return func(res rest.ResponseWriter, req *rest.Request) {
if handlerFunc != nil && res != nil && req != nil {
Expand All @@ -34,6 +49,7 @@ func RequireServer(handlerFunc rest.HandlerFunc) rest.HandlerFunc {
}
}

// RequireUser aborts with an error if a request isn't authenticated as a user.
func RequireUser(handlerFunc rest.HandlerFunc) rest.HandlerFunc {
return func(res rest.ResponseWriter, req *rest.Request) {
if handlerFunc != nil && res != nil && req != nil {
Expand Down
6 changes: 3 additions & 3 deletions service/api/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
testRest "github.com/tidepool-org/platform/test/rest"
)

var _ = Describe("Auth", func() {
var _ = Describe("Authenticator", func() {
var res *testRest.ResponseWriter
var req *rest.Request
var handlerFunc rest.HandlerFunc
Expand Down Expand Up @@ -46,7 +46,7 @@ var _ = Describe("Auth", func() {

Context("Require", func() {
It("does nothing if handlerFunc is nil", func() {
requireFunc := api.Require(nil)
requireFunc := api.RequireAuth(nil)
Expect(requireFunc).ToNot(BeNil())
requireFunc(res, req)
Expect(res.WriteHeaderInputs).To(BeEmpty())
Expand All @@ -57,7 +57,7 @@ var _ = Describe("Auth", func() {
var requireFunc rest.HandlerFunc

BeforeEach(func() {
requireFunc = api.Require(handlerFunc)
requireFunc = api.RequireAuth(handlerFunc)
Expect(requireFunc).ToNot(BeNil())
})

Expand Down
Loading

0 comments on commit 9088da2

Please sign in to comment.