fix: authentication flow should abort early (#888)

* fix: finish connection once we send auth response

* removed interface for now

* handle authentication in each route group

* tags api tests

* typo

* testutil improvements

* bookmarks api auth

* cache update requires owner

internal/http/middleware/auth_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/gin-gonic/gin"
+	"github.com/go-shiori/shiori/internal/http/response"
 	"github.com/go-shiori/shiori/internal/model"
 	"github.com/go-shiori/shiori/internal/testutil"
 	"github.com/sirupsen/logrus"
@@ -18,8 +19,13 @@ func TestAuthenticationRequiredMiddleware(t *testing.T) {
 	t.Run("test unauthorized", func(t *testing.T) {
 		g := testutil.NewGin()
 		g.Use(AuthenticationRequired())
+		g.Handle("GET", "/", func(c *gin.Context) {
+			response.Send(c, http.StatusOK, nil)
+		})
 		w := testutil.PerformRequest(g, "GET", "/")
 		require.Equal(t, http.StatusUnauthorized, w.Code)
+		// This ensures we are aborting the request and not sending more data
+		require.Equal(t, `{"ok":false,"message":null}`, w.Body.String())
 	})
 
 	t.Run("test authorized", func(t *testing.T) {

internal/http/response/response.go

@@ -10,7 +10,7 @@ type Response struct {
 	Ok bool `json:"ok"`
 
 	// Message the payload of the response, depending on the endpoint/response status
-	Message interface{} `json:"message"`
+	Message any `json:"message"`
 
 	// ErrorParams parameters defined if the response is not successful to help client's debugging
 	ErrorParams map[string]string `json:"error_params,omitempty"`
@@ -20,15 +20,19 @@ type Response struct {
 }
 
 func (m *Response) IsError() bool {
-	return m.Ok
+	return !m.Ok
+}
+
+func (m *Response) GetMessage() any {
+	return m.Message
 }
 
 func (m *Response) Send(c *gin.Context) {
 	c.Status(m.statusCode)
 	c.JSON(m.statusCode, m)
 }
 
-func NewResponse(ok bool, message interface{}, errorParams map[string]string, statusCode int) *Response {
+func NewResponse(ok bool, message any, errorParams map[string]string, statusCode int) *Response {
 	return &Response{
 		Ok:          ok,
 		Message:     message,

internal/http/response/shortcuts.go

@@ -21,6 +21,7 @@ func Send(ctx *gin.Context, statusCode int, data interface{}) {
 // SendError provides a shortcut to send an unsuccessful response
 func SendError(ctx *gin.Context, statusCode int, data interface{}) {
 	New(false, statusCode, data).Send(ctx)
+	ctx.Abort()
 }
 
 // SendErrorWithParams the same as above but for errors that require error parameters

internal/http/routes/api/v1/api.go

@@ -3,7 +3,6 @@ package api_v1
 import (
 	"github.com/gin-gonic/gin"
 	"github.com/go-shiori/shiori/internal/dependencies"
-	"github.com/go-shiori/shiori/internal/http/middleware"
 	"github.com/go-shiori/shiori/internal/model"
 	"github.com/sirupsen/logrus"
 )
@@ -17,10 +16,7 @@ type APIRoutes struct {
 func (r *APIRoutes) Setup(g *gin.RouterGroup) model.Routes {
 	// Account API handles authentication in each route
 	r.handle(g, "/auth", NewAuthAPIRoutes(r.logger, r.deps, r.loginHandler))
-
-	// From here on, all routes require authentication
-	g.Use(middleware.AuthenticationRequired())
-	r.handle(g, "/bookmarks", NewBookmarksPIRoutes(r.logger, r.deps))
+	r.handle(g, "/bookmarks", NewBookmarksAPIRoutes(r.logger, r.deps))
 	r.handle(g, "/tags", NewTagsPIRoutes(r.logger, r.deps))
 
 	return r

internal/http/routes/api/v1/bookmarks.go

@@ -13,6 +13,7 @@ import (
 	"github.com/go-shiori/shiori/internal/database"
 	"github.com/go-shiori/shiori/internal/dependencies"
 	"github.com/go-shiori/shiori/internal/http/context"
+	"github.com/go-shiori/shiori/internal/http/middleware"
 	"github.com/go-shiori/shiori/internal/http/response"
 	"github.com/go-shiori/shiori/internal/model"
 	"github.com/sirupsen/logrus"
@@ -24,11 +25,12 @@ type BookmarksAPIRoutes struct {
 }
 
 func (r *BookmarksAPIRoutes) Setup(g *gin.RouterGroup) model.Routes {
+	g.Use(middleware.AuthenticationRequired())
 	g.PUT("/cache", r.updateCache)
 	return r
 }
 
-func NewBookmarksPIRoutes(logger *logrus.Logger, deps *dependencies.Dependencies) *BookmarksAPIRoutes {
+func NewBookmarksAPIRoutes(logger *logrus.Logger, deps *dependencies.Dependencies) *BookmarksAPIRoutes {
 	return &BookmarksAPIRoutes{
 		logger: logger,
 		deps:   deps,
@@ -67,7 +69,7 @@ func (p *updateCachePayload) IsValid() error {
 //	@Router						/api/v1/bookmarks/cache [put]
 func (r *BookmarksAPIRoutes) updateCache(c *gin.Context) {
 	ctx := context.NewContextFromGin(c)
-	if !ctx.UserIsLogged() {
+	if !ctx.GetAccount().Owner {
 		response.SendError(c, http.StatusForbidden, nil)
 		return
 	}
@@ -185,7 +187,7 @@ func (r *BookmarksAPIRoutes) updateCache(c *gin.Context) {
 	close(chDone)
 
 	// Update database
-	_, err = r.deps.Database.SaveBookmarks(ctx, false, bookmarks...)
+	_, err = r.deps.Database.SaveBookmarks(c, false, bookmarks...)
 	if err != nil {
 		r.logger.WithError(err).Error("error update bookmakrs on deatabas")
 		response.SendInternalServerError(c)

internal/http/routes/api/v1/bookmarks_test.go

@@ -1 +1,47 @@
 package api_v1
+
+import (
+	"context"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/gin-gonic/gin"
+	"github.com/go-shiori/shiori/internal/http/middleware"
+	"github.com/go-shiori/shiori/internal/model"
+	"github.com/go-shiori/shiori/internal/testutil"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/require"
+)
+
+func TestUpdateBookmarkCache(t *testing.T) {
+	logger := logrus.New()
+	ctx := context.TODO()
+
+	g := gin.New()
+
+	_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
+	g.Use(middleware.AuthMiddleware(deps))
+
+	router := NewBookmarksAPIRoutes(logger, deps)
+	router.Setup(g.Group("/"))
+
+	account := model.Account{
+		Username: "test",
+		Password: "test",
+		Owner:    false,
+	}
+	require.NoError(t, deps.Database.SaveAccount(ctx, account))
+	token, err := deps.Domains.Auth.CreateTokenForAccount(&account, time.Now().Add(time.Minute))
+	require.NoError(t, err)
+
+	t.Run("require authentication", func(t *testing.T) {
+		w := testutil.PerformRequest(g, "PUT", "/cache")
+		require.Equal(t, http.StatusUnauthorized, w.Code)
+	})
+
+	t.Run("require owner", func(t *testing.T) {
+		w := testutil.PerformRequest(g, "PUT", "/cache", testutil.WithHeader(model.AuthorizationHeader, model.AuthorizationTokenType+" "+token))
+		require.Equal(t, http.StatusForbidden, w.Code)
+	})
+}

internal/http/routes/api/v1/tags.go

@@ -5,6 +5,8 @@ import (
 
 	"github.com/gin-gonic/gin"
 	"github.com/go-shiori/shiori/internal/dependencies"
+	"github.com/go-shiori/shiori/internal/http/context"
+	"github.com/go-shiori/shiori/internal/http/middleware"
 	"github.com/go-shiori/shiori/internal/http/response"
 	"github.com/go-shiori/shiori/internal/model"
 	"github.com/sirupsen/logrus"
@@ -16,6 +18,7 @@ type TagsAPIRoutes struct {
 }
 
 func (r *TagsAPIRoutes) Setup(g *gin.RouterGroup) model.Routes {
+	g.Use(middleware.AuthenticationRequired())
 	g.GET("/", r.listHandler)
 	g.POST("/", r.createHandler)
 	return r
@@ -47,6 +50,12 @@ func (r *TagsAPIRoutes) listHandler(c *gin.Context) {
 // @Failure					403	{object}	nil			"Token not provided/invalid"
 // @Router						/api/v1/tags [post]
 func (r *TagsAPIRoutes) createHandler(c *gin.Context) {
+	ctx := context.NewContextFromGin(c)
+	if !ctx.GetAccount().Owner {
+		response.SendError(c, http.StatusForbidden, nil)
+		return
+	}
+
 	var tag model.Tag
 	if err := c.BindJSON(&tag); err != nil {
 		response.SendError(c, http.StatusBadRequest, nil)

internal/http/routes/api/v1/tags_test.go

@@ -4,8 +4,11 @@ import (
 	"context"
 	"net/http"
 	"testing"
+	"time"
 
 	"github.com/gin-gonic/gin"
+	"github.com/go-shiori/shiori/internal/http/middleware"
+	"github.com/go-shiori/shiori/internal/model"
 	"github.com/go-shiori/shiori/internal/testutil"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
@@ -15,34 +18,81 @@ func TestTagList(t *testing.T) {
 	logger := logrus.New()
 	ctx := context.TODO()
 
-	t.Run("empty tag list", func(t *testing.T) {
-		g := gin.New()
-		_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
-		router := NewTagsPIRoutes(logger, deps)
-		router.Setup(g.Group("/"))
+	g := gin.New()
+
+	_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
+	g.Use(middleware.AuthMiddleware(deps))
+
+	account := model.Account{
+		Username: "test",
+		Password: "test",
+		Owner:    true,
+	}
+	require.NoError(t, deps.Database.SaveAccount(ctx, account))
+	token, err := deps.Domains.Auth.CreateTokenForAccount(&account, time.Now().Add(time.Minute))
+	require.NoError(t, err)
+
+	bookmark := testutil.GetValidBookmark()
+	bookmark.Tags = []model.Tag{
+		{Name: "test"},
+	}
+	_, err = deps.Database.SaveBookmarks(ctx, true, *bookmark)
+	require.NoError(t, err)
+
+	router := NewTagsPIRoutes(logger, deps)
+	router.Setup(g.Group("/"))
+
+	t.Run("require authentication", func(t *testing.T) {
 		w := testutil.PerformRequest(g, "GET", "/")
-		require.Equal(t, http.StatusOK, w.Code)
+		require.Equal(t, http.StatusUnauthorized, w.Code)
 
 		response, err := testutil.NewTestResponseFromReader(w.Body)
 		require.NoError(t, err)
 
-		response.AssertMessageIsEmptyList(t)
+		response.AssertNotOk(t)
 	})
 
 	t.Run("return tags", func(t *testing.T) {
-		ctx := context.TODO()
-
-		g := gin.New()
-		_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
-
-		router := NewTagsPIRoutes(logger, deps)
-		router.Setup(g.Group("/"))
-		w := testutil.PerformRequest(g, "GET", "/")
+		w := testutil.PerformRequest(g, "GET", "/", testutil.WithHeader(model.AuthorizationHeader, model.AuthorizationTokenType+" "+token))
 		require.Equal(t, http.StatusOK, w.Code)
 
 		response, err := testutil.NewTestResponseFromReader(w.Body)
 		require.NoError(t, err)
 
-		response.AssertMessageIsEmptyList(t)
+		response.AssertOk(t)
+		response.AssertMessageIsListLength(t, 1)
+	})
+}
+
+func TestTagCreate(t *testing.T) {
+	logger := logrus.New()
+	ctx := context.TODO()
+
+	g := gin.New()
+
+	_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
+	g.Use(middleware.AuthMiddleware(deps))
+
+	account := model.Account{
+		Username: "test",
+		Password: "test",
+		Owner:    true,
+	}
+	require.NoError(t, deps.Database.SaveAccount(ctx, account))
+	// token, err := deps.Domains.Auth.CreateTokenForAccount(&account, time.Now().Add(time.Minute))
+	// require.NoError(t, err)
+
+	router := NewTagsPIRoutes(logger, deps)
+	router.Setup(g.Group("/"))
+
+	t.Run("require authentication", func(t *testing.T) {
+		w := testutil.PerformRequest(g, "POST", "/")
+		require.Equal(t, http.StatusUnauthorized, w.Code)
+	})
+
+	t.Run("create tag", func(t *testing.T) {
+		// TODO: Implement this test
+		// Tags require a bookmark to be created, so we need to create a bookmark first
+		// but I'm not sure if we should enforce this.
 	})
 }

internal/testutil/response.go

@@ -11,42 +11,50 @@ import (
 )
 
 type testResponse struct {
-	Response *response.Response
+	Response response.Response
 }
 
 func (r *testResponse) AssertMessageIsEmptyList(t *testing.T) {
-	require.Equal(t, []interface{}{}, r.Response.Message)
+	require.Equal(t, []interface{}{}, r.Response.GetMessage())
 }
 
 func (r *testResponse) AssertNilMessage(t *testing.T) {
-	require.Equal(t, nil, r.Response.Message)
+	require.Equal(t, nil, r.Response.GetMessage())
 }
 
 func (r testResponse) AssertMessageEquals(t *testing.T, expected interface{}) {
-	require.Equal(t, expected, r.Response.Message)
+	require.Equal(t, expected, r.Response.GetMessage())
+}
+
+func (r *testResponse) AssertMessageIsListLength(t *testing.T, length int) {
+	require.Len(t, r.Response.GetMessage(), length)
 }
 
 func (r *testResponse) AssertOk(t *testing.T) {
-	require.True(t, r.Response.Ok)
+	require.False(t, r.Response.IsError())
 }
 
 func (r *testResponse) AssertNotOk(t *testing.T) {
-	require.False(t, r.Response.Ok)
+	require.True(t, r.Response.IsError())
+}
+
+func (r *testResponse) Assert(t *testing.T, fn func(t *testing.T, r *testResponse)) {
+	fn(t, r)
 }
 
 func NewTestResponseFromBytes(b []byte) (*testResponse, error) {
-	r := testResponse{}
-	if err := json.Unmarshal(b, &r.Response); err != nil {
+	tr := testResponse{}
+	if err := json.Unmarshal(b, &tr.Response); err != nil {
 		return nil, errors.Wrap(err, "error parsing response")
 	}
-	return &r, nil
+	return &tr, nil
 }
 
 func NewTestResponseFromReader(r io.Reader) (*testResponse, error) {
-	response := testResponse{}
+	tr := testResponse{}
 	decoder := json.NewDecoder(r)
-	if err := decoder.Decode(&response.Response); err != nil {
+	if err := decoder.Decode(&tr.Response); err != nil {
 		return nil, errors.Wrap(err, "error parsing response")
 	}
-	return &response, nil
+	return &tr, nil
 }