Skip to content

Commit 514df1e

Browse files
fmartingrfmartingr
authored
fix: auth validation on existing sessions, rely on token only (#1069)
* chore: use http.NoBody * fix: remove cookie token on logout * fix: remove token cookie on middleware and redirect * fix: frontend sets cookie token if authenticated * refactor: remove session-id, rely on token only * docs: make swagger * fix: redirect * fix: archive route handler * fix: properly unset cookie
1 parent 876d27f commit 514df1e

File tree

17 files changed

+124
-169
lines changed

17 files changed

+124
-169
lines changed

‎docs/swagger/docs.go‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -439,13 +439,8 @@ const docTemplate = `{
439439
"type": "object",
440440
"properties": {
441441
"expires": {
442-
"description": "Deprecated, used only for legacy APIs",
443442
"type": "integer"
444443
},
445-
"session": {
446-
"description": "Deprecated, used only for legacy APIs",
447-
"type": "string"
448-
},
449444
"token": {
450445
"type": "string"
451446
}

‎docs/swagger/swagger.json‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -428,13 +428,8 @@
428428
"type": "object",
429429
"properties": {
430430
"expires": {
431-
"description": "Deprecated, used only for legacy APIs",
432431
"type": "integer"
433432
},
434-
"session": {
435-
"description": "Deprecated, used only for legacy APIs",
436-
"type": "string"
437-
},
438433
"token": {
439434
"type": "string"
440435
}

‎docs/swagger/swagger.yaml‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,7 @@ definitions:
2727
api_v1.loginResponseMessage:
2828
properties:
2929
expires:
30-
description: Deprecated, used only for legacy APIs
3130
type: integer
32-
session:
33-
description: Deprecated, used only for legacy APIs
34-
type: string
3531
token:
3632
type: string
3733
type: object

‎internal/http/handlers/api/v1/accounts_test.go‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -357,9 +357,7 @@ func TestHandleUpdateAccount(t *testing.T) {
357357

358358
// Verify we can login with new password
359359
loginBody := `{"username": "shiori", "password": "newpass"}`
360-
w = testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
361-
HandleLogin(deps, c, noopLegacyLoginHandler)
362-
}, "POST", "/login", testutil.WithBody(loginBody))
360+
w = testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(loginBody))
363361
require.Equal(t, http.StatusOK, w.Code)
364362
})
365363

@@ -480,9 +478,7 @@ func TestHandleUpdateAccount(t *testing.T) {
480478

481479
// Verify password change
482480
loginBody := `{"username": "updated", "password": "newpass"}`
483-
w = testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
484-
HandleLogin(deps, c, noopLegacyLoginHandler)
485-
}, "POST", "/login", testutil.WithBody(loginBody))
481+
w = testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(loginBody))
486482
require.Equal(t, http.StatusOK, w.Code)
487483
})
488484
}

‎internal/http/handlers/api/v1/auth.go‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,7 @@ func (p *loginRequestPayload) IsValid() error {
2929

3030
type loginResponseMessage struct {
3131
Token string `json:"token"`
32-
SessionID string `json:"session"` // Deprecated, used only for legacy APIs
33-
Expiration int64 `json:"expires"` // Deprecated, used only for legacy APIs
32+
Expiration int64 `json:"expires"`
3433
}
3534

3635
// @Summary Login to an account using username and password
@@ -41,7 +40,7 @@ type loginResponseMessage struct {
4140
// @Success 200 {object} loginResponseMessage "Login successful"
4241
// @Failure 400 {object} nil "Invalid login data"
4342
// @Router /api/v1/auth/login [post]
44-
func HandleLogin(deps model.Dependencies, c model.WebContext, legacyLoginHandler model.LegacyLoginHandler) {
43+
func HandleLogin(deps model.Dependencies, c model.WebContext) {
4544
var payload loginRequestPayload
4645
if err := json.NewDecoder(c.Request().Body).Decode(&payload); err != nil {
4746
response.SendError(c, http.StatusBadRequest, "Invalid JSON payload", nil)
@@ -72,16 +71,8 @@ func HandleLogin(deps model.Dependencies, c model.WebContext, legacyLoginHandler
7271
return
7372
}
7473

75-
sessionID, err := legacyLoginHandler(account, expiration)
76-
if err != nil {
77-
deps.Logger().WithError(err).Error("failed execute legacy login handler")
78-
response.SendInternalServerError(c)
79-
return
80-
}
81-
8274
response.Send(c, http.StatusOK, loginResponseMessage{
8375
Token: token,
84-
SessionID: sessionID,
8576
Expiration: expirationTime.Unix(),
8677
})
8778
}
@@ -215,5 +206,12 @@ func HandleLogout(deps model.Dependencies, c model.WebContext) {
215206
if err := middleware.RequireLoggedInUser(deps, c); err != nil {
216207
return
217208
}
209+
210+
// Remove token cookie
211+
c.Request().AddCookie(&http.Cookie{
212+
Name: "token",
213+
Value: "",
214+
})
215+
218216
response.Send(c, http.StatusOK, nil)
219217
}

‎internal/http/handlers/api/v1/auth_test.go‎

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,13 @@ import (
44
"context"
55
"net/http"
66
"testing"
7-
"time"
87

98
"github.com/go-shiori/shiori/internal/model"
109
"github.com/go-shiori/shiori/internal/testutil"
1110
"github.com/sirupsen/logrus"
1211
"github.com/stretchr/testify/require"
1312
)
1413

15-
func noopLegacyLoginHandler(_ *model.AccountDTO, _ time.Duration) (string, error) {
16-
return "test-session", nil
17-
}
18-
1914
func TestHandleLogin(t *testing.T) {
2015
logger := logrus.New()
2116
// _, deps := testutil.GetTestConfigurationAndDependencies(t, context.Background(), logger)
@@ -24,39 +19,31 @@ func TestHandleLogin(t *testing.T) {
2419
ctx := context.Background()
2520
_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
2621
body := `{"username":}`
27-
w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
28-
HandleLogin(deps, c, noopLegacyLoginHandler)
29-
}, "POST", "/login", testutil.WithBody(body))
22+
w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
3023
require.Equal(t, http.StatusBadRequest, w.Code)
3124
})
3225

3326
t.Run("missing username", func(t *testing.T) {
3427
ctx := context.Background()
3528
_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
3629
body := `{"password": "test"}`
37-
w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
38-
HandleLogin(deps, c, noopLegacyLoginHandler)
39-
}, "POST", "/login", testutil.WithBody(body))
30+
w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
4031
require.Equal(t, http.StatusBadRequest, w.Code)
4132
})
4233

4334
t.Run("missing password", func(t *testing.T) {
4435
ctx := context.Background()
4536
_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
4637
body := `{"username": "test"}`
47-
w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
48-
HandleLogin(deps, c, noopLegacyLoginHandler)
49-
}, "POST", "/login", testutil.WithBody(body))
38+
w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
5039
require.Equal(t, http.StatusBadRequest, w.Code)
5140
})
5241

5342
t.Run("invalid credentials", func(t *testing.T) {
5443
ctx := context.Background()
5544
_, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger)
5645
body := `{"username": "test", "password": "wrong"}`
57-
w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
58-
HandleLogin(deps, c, noopLegacyLoginHandler)
59-
}, "POST", "/login", testutil.WithBody(body))
46+
w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
6047
require.Equal(t, http.StatusBadRequest, w.Code)
6148
})
6249

@@ -74,16 +61,13 @@ func TestHandleLogin(t *testing.T) {
7461
"password": "test",
7562
"remember_me": true
7663
}`
77-
w := testutil.PerformRequest(deps, func(deps model.Dependencies, c model.WebContext) {
78-
HandleLogin(deps, c, noopLegacyLoginHandler)
79-
}, "POST", "/login", testutil.WithBody(body))
64+
w := testutil.PerformRequest(deps, HandleLogin, "POST", "/login", testutil.WithBody(body))
8065
require.Equal(t, http.StatusOK, w.Code)
8166

8267
response, err := testutil.NewTestResponseFromReader(w.Body)
8368
require.NoError(t, err)
8469
response.AssertOk(t)
8570
response.AssertMessageContains(t, "token")
86-
response.AssertMessageContains(t, "session")
8771
response.AssertMessageContains(t, "expires")
8872
})
8973
}

‎internal/http/handlers/bookmark.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"html/template"
66
"net/http"
77
"strconv"
8-
"strings"
98

109
"github.com/go-shiori/shiori/internal/http/response"
1110
"github.com/go-shiori/shiori/internal/model"
@@ -92,8 +91,7 @@ func HandleBookmarkArchiveFile(deps model.Dependencies, c model.WebContext) {
9291
return
9392
}
9493

95-
// Get resource path from URL
96-
resourcePath := strings.TrimPrefix(c.Request().URL.Path, fmt.Sprintf("/bookmark/%d/archive/file/", bookmark.ID))
94+
resourcePath := c.Request().PathValue("path")
9795

9896
archive, err := deps.Domains().Archiver().GetBookmarkArchive(bookmark)
9997
if err != nil {

‎internal/http/handlers/legacy.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,18 @@ func (h *LegacyHandler) HandleLogin(account *model.AccountDTO, expTime time.Dura
5050
}
5151

5252
strSessionID := sessionID.String()
53-
h.legacyHandler.SessionCache.Set(strSessionID, account, expTime)
5453

5554
return strSessionID, nil
5655
}
5756

5857
// HandleLogout handles the legacy logout endpoint
5958
func (h *LegacyHandler) HandleLogout(deps model.Dependencies, c model.WebContext) {
60-
sessionID := h.legacyHandler.GetSessionID(c.Request())
61-
h.legacyHandler.SessionCache.Delete(sessionID)
59+
// TODO: Leave cookie handling to API consumer or middleware?
60+
// Remove token cookie
61+
c.Request().AddCookie(&http.Cookie{
62+
Name: "token",
63+
Value: "",
64+
})
6265
}
6366

6467
// HandleGetTags handles GET /api/tags

‎internal/http/handlers/legacy_test.go‎

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -36,30 +36,6 @@ func TestLegacyHandler(t *testing.T) {
3636
sessionID, err := handler.HandleLogin(account, time.Hour)
3737
require.NoError(t, err)
3838
require.NotEmpty(t, sessionID)
39-
40-
// Verify session is stored
41-
val, found := handler.legacyHandler.SessionCache.Get(sessionID)
42-
require.True(t, found)
43-
require.Equal(t, account, val)
44-
})
45-
46-
t.Run("HandleLogout", func(t *testing.T) {
47-
// Setup session
48-
account := &model.AccountDTO{ID: 1}
49-
sessionID, _ := handler.HandleLogin(account, time.Hour)
50-
51-
// Create request with session cookie
52-
c, _ := testutil.NewTestWebContext()
53-
c.Request().AddCookie(&http.Cookie{
54-
Name: "session-id",
55-
Value: sessionID,
56-
})
57-
58-
handler.HandleLogout(deps, c)
59-
60-
// Verify session is removed
61-
_, found := handler.legacyHandler.SessionCache.Get(sessionID)
62-
require.False(t, found)
6339
})
6440

6541
t.Run("HandleGetTags", func(t *testing.T) {
@@ -79,7 +55,7 @@ func TestLegacyHandler(t *testing.T) {
7955
})
8056

8157
t.Run("convertParams", func(t *testing.T) {
82-
r, _ := http.NewRequest(http.MethodGet, "/api/bookmarks?page=1&tags=test,dev", nil)
58+
r, _ := http.NewRequest(http.MethodGet, "/api/bookmarks?page=1&tags=test,dev", http.NoBody)
8359
params := handler.convertParams(r)
8460

8561
require.Len(t, params, 2)

‎internal/http/middleware/auth.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,14 @@ func (m *AuthMiddleware) OnRequest(deps model.Dependencies, c model.WebContext)
3232

3333
account, err := deps.Domains().Auth().CheckToken(c.Request().Context(), token)
3434
if err != nil {
35-
deps.Logger().WithError(err).Error("Failed to check token")
36-
return err
35+
// If we fail to check token, remove the token cookie and redirect to login
36+
deps.Logger().WithError(err).WithField("request_id", c.GetRequestID()).Error("Failed to check token")
37+
http.SetCookie(c.ResponseWriter(), &http.Cookie{
38+
Name: "token",
39+
Value: "",
40+
MaxAge: -1,
41+
})
42+
return nil
3743
}
3844

3945
c.SetAccount(account)

0 commit comments

Comments
 (0)