From 9994bed47e034aef99497ca3abb95f996cbbdf15 Mon Sep 17 00:00:00 2001 From: Haelnorr Date: Sun, 16 Feb 2025 13:54:44 +1100 Subject: [PATCH 1/3] Fixed nil pointer derefence due accessing account page if cookie not set --- handlers/account.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/handlers/account.go b/handlers/account.go index be79910..365a283 100644 --- a/handlers/account.go +++ b/handlers/account.go @@ -19,9 +19,9 @@ func HandleAccountPage() http.Handler { return http.HandlerFunc( func(w http.ResponseWriter, r *http.Request) { cookie, err := r.Cookie("subpage") - subpage := cookie.Value - if err != nil { - subpage = "General" + subpage := "General" + if err == nil { + subpage = cookie.Value } page.Account(subpage).Render(r.Context(), w) }, From deaeddb53332ee26a3c0b2253a7b478ad80123d7 Mon Sep 17 00:00:00 2001 From: Haelnorr Date: Sun, 16 Feb 2025 14:16:32 +1100 Subject: [PATCH 2/3] Made authentication test more robust --- Makefile | 1 + middleware/authentication_test.go | 76 ++++++++++++++++++------------- testdata.sql | 4 +- 3 files changed, 48 insertions(+), 33 deletions(-) diff --git a/Makefile b/Makefile index c056648..a2f26e2 100644 --- a/Makefile +++ b/Makefile @@ -19,6 +19,7 @@ tester: go run . --port 3232 --test --loglevel trace test: + rm -f **/.projectreshoot-test-database.db go mod tidy && \ go test . -v go test ./middleware -v diff --git a/middleware/authentication_test.go b/middleware/authentication_test.go index 8465ca0..a5143c8 100644 --- a/middleware/authentication_test.go +++ b/middleware/authentication_test.go @@ -8,8 +8,6 @@ import ( "testing" "projectreshoot/contexts" - "projectreshoot/db" - "projectreshoot/jwt" "projectreshoot/tests" "github.com/stretchr/testify/assert" @@ -45,27 +43,7 @@ func TestAuthenticationMiddleware(t *testing.T) { server := httptest.NewServer(authHandler) defer server.Close() - // Setup the user and tokens to test with - user, err := db.GetUserFromID(conn, 1) - require.NoError(t, err) - - // Good tokens - atStr, _, err := jwt.GenerateAccessToken(cfg, user, false, false) - require.NoError(t, err) - rtStr, _, err := jwt.GenerateRefreshToken(cfg, user, false) - require.NoError(t, err) - - // Create a token and revoke it for testing - expStr, _, err := jwt.GenerateAccessToken(cfg, user, false, false) - require.NoError(t, err) - expT, err := jwt.ParseAccessToken(cfg, conn, expStr) - require.NoError(t, err) - err = jwt.RevokeToken(conn, expT) - require.NoError(t, err) - - // Make sure it actually got revoked - expT, err = jwt.ParseAccessToken(cfg, conn, expStr) - require.Error(t, err) + tokens := getTokens() tests := []struct { name string @@ -75,29 +53,48 @@ func TestAuthenticationMiddleware(t *testing.T) { expectedCode int }{ { - name: "Valid Access Token", + name: "Valid Access Token (Fresh)", id: 1, - accessToken: atStr, + accessToken: tokens["accessFresh"], refreshToken: "", expectedCode: http.StatusOK, }, + { + name: "Valid Access Token (Unfresh)", + id: 1, + accessToken: tokens["accessUnfresh"], + refreshToken: tokens["refreshExpired"], + expectedCode: http.StatusOK, + }, { name: "Valid Refresh Token (Triggers Refresh)", id: 1, - accessToken: expStr, - refreshToken: rtStr, + accessToken: tokens["accessExpired"], + refreshToken: tokens["refreshValid"], expectedCode: http.StatusOK, }, { - name: "Refresh token revoked (after refresh)", - accessToken: expStr, - refreshToken: rtStr, + name: "Both tokens expired", + accessToken: tokens["accessExpired"], + refreshToken: tokens["refreshExpired"], + expectedCode: http.StatusUnauthorized, + }, + { + name: "Access token revoked", + accessToken: tokens["accessRevoked"], + refreshToken: "", + expectedCode: http.StatusUnauthorized, + }, + { + name: "Refresh token revoked", + accessToken: "", + refreshToken: tokens["refreshRevoked"], expectedCode: http.StatusUnauthorized, }, { name: "Invalid Tokens", - accessToken: expStr, - refreshToken: expStr, + accessToken: tokens["invalid"], + refreshToken: tokens["invalid"], expectedCode: http.StatusUnauthorized, }, { @@ -130,3 +127,18 @@ func TestAuthenticationMiddleware(t *testing.T) { }) } } + +// get the tokens to test with +func getTokens() map[string]string { + tokens := map[string]string{ + "accessFresh": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjQ4OTU2NzIyMTAsImZyZXNoIjo0ODk1NjcyMjEwLCJpYXQiOjE3Mzk2NzIyMTAsImlzcyI6IjEyNy4wLjAuMSIsImp0aSI6ImE4Njk2YWM4LTg3OWMtNDdkNC1iZWM2LTRlY2Y4MTRiZThiZiIsInNjb3BlIjoiYWNjZXNzIiwic3ViIjoxLCJ0dGwiOiJzZXNzaW9uIn0.6nAquDY0JBLPdaJ9q_sMpKj1ISG4Vt2U05J57aoPue8", + "accessUnfresh": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjMzMjk5Njc1NjcxLCJmcmVzaCI6MTczOTY3NTY3MSwiaWF0IjoxNzM5Njc1NjcxLCJpc3MiOiIxMjcuMC4wLjEiLCJqdGkiOiJjOGNhZmFjNy0yODkzLTQzNzMtOTI4ZS03MGUwODJkYmM2MGIiLCJzY29wZSI6ImFjY2VzcyIsInN1YiI6MSwidHRsIjoic2Vzc2lvbiJ9.plWQVFwHlhXUYI5utS7ny1JfXjJSFrigkq-PnTHD5VY", + "accessExpired": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE3Mzk2NzIyNDgsImZyZXNoIjoxNzM5NjcyMjQ4LCJpYXQiOjE3Mzk2NzIyNDgsImlzcyI6IjEyNy4wLjAuMSIsImp0aSI6IjgxYzA1YzBjLTJhOGItNGQ2MC04Yzc4LWY2ZTQxODYxZDFmNCIsInNjb3BlIjoiYWNjZXNzIiwic3ViIjoxLCJ0dGwiOiJzZXNzaW9uIn0.iI1f17kKTuFDEMEYltJRIwRYgYQ-_nF9Wsn0KR6x77Q", + "refreshValid": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjQ4OTU2NzE5MjIsImlhdCI6MTczOTY3MTkyMiwiaXNzIjoiMTI3LjAuMC4xIiwianRpIjoiZTUxMTY3ZWEtNDA3OS00ZTczLTkzZDQtNTgwZDMzODRjZDU4Iiwic2NvcGUiOiJyZWZyZXNoIiwic3ViIjoxLCJ0dGwiOiJzZXNzaW9uIn0.tvtqQ8Z4WrYWHHb0MaEPdsU2FT2KLRE1zHOv3ipoFyc", + "refreshExpired": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE3Mzk2NzIyNDgsImlhdCI6MTczOTY3MjI0OCwiaXNzIjoiMTI3LjAuMC4xIiwianRpIjoiZTg5YTc5MTYtZGEzYi00YmJhLWI3ZDMtOWI1N2ViNjRhMmU0Iiwic2NvcGUiOiJyZWZyZXNoIiwic3ViIjoxLCJ0dGwiOiJzZXNzaW9uIn0.rH_fytC7Duxo598xacu820pQKF9ELbG8674h_bK_c4I", + "accessRevoked": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjQ4OTU2NzE5MjIsImZyZXNoIjoxNzM5NjcxOTIyLCJpYXQiOjE3Mzk2NzE5MjIsImlzcyI6IjEyNy4wLjAuMSIsImp0aSI6IjBhNmIzMzhlLTkzMGEtNDNmZS04ZjcwLTFhNmRhZWQyNTZmYSIsInNjb3BlIjoiYWNjZXNzIiwic3ViIjoxLCJ0dGwiOiJzZXNzaW9uIn0.mZLuCp9amcm2_CqYvbHPlk86nfiuy_Or8TlntUCw4Qs", + "refreshRevoked": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjMzMjk5Njc1NjcxLCJpYXQiOjE3Mzk2NzU2NzEsImlzcyI6IjEyNy4wLjAuMSIsImp0aSI6ImI3ZmE1MWRjLTg1MzItNDJlMS04NzU2LTVkMjViZmIyMDAzYSIsInNjb3BlIjoicmVmcmVzaCIsInN1YiI6MSwidHRsIjoic2Vzc2lvbiJ9.5Q9yDZN5FubfCWHclUUZEkJPOUHcOEpVpgcUK-ameHo", + "invalid": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE0ODUxNDA5ODQsImlhdCI6MTQ4NTEzNzM4NCwiaXNzIjoiYWNtZS5jb20iLCJzdWIiOiIyOWFjMGMxOC0wYjRhLTQyY2YtODJmYy0wM2Q1NzAzMThhMWQiLCJhcHBsaWNhdGlvbklkIjoiNzkxMDM3MzQtOTdhYi00ZDFhLWFmMzctZTAwNmQwNWQyOTUyIiwicm9sZXMiOltdfQ.Mp0Pcwsz5VECK11Kf2ZZNF_SMKu5CgBeLN9ZOP04kZo", + } + return tokens +} diff --git a/testdata.sql b/testdata.sql index 6b05c97..be1ee04 100644 --- a/testdata.sql +++ b/testdata.sql @@ -1 +1,3 @@ -INSERT INTO users VALUES(1,'testuser','hashedpassword',1738995274); +INSERT INTO users VALUES(1,'testuser','hashedpassword',1738995274, 'bio'); +INSERT INTO jwtblacklist VALUES('0a6b338e-930a-43fe-8f70-1a6daed256fa', 33299675344); +INSERT INTO jwtblacklist VALUES('b7fa51dc-8532-42e1-8756-5d25bfb2003a', 33299675344); From 31f1b83da424b302e4c9fbaecf9d775f119b3983 Mon Sep 17 00:00:00 2001 From: Haelnorr Date: Sun, 16 Feb 2025 15:15:52 +1100 Subject: [PATCH 3/3] Added page protection and reauthentication tests --- Makefile | 4 +- middleware/pageprotection.go | 1 + middleware/pageprotection_test.go | 81 ++++++++++++++++++++++++++ middleware/reauthentication_test.go | 88 +++++++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 2 deletions(-) create mode 100644 middleware/pageprotection_test.go create mode 100644 middleware/reauthentication_test.go diff --git a/Makefile b/Makefile index a2f26e2..68d12a7 100644 --- a/Makefile +++ b/Makefile @@ -21,8 +21,8 @@ tester: test: rm -f **/.projectreshoot-test-database.db go mod tidy && \ - go test . -v - go test ./middleware -v + go test . + go test ./middleware clean: go clean diff --git a/middleware/pageprotection.go b/middleware/pageprotection.go index 64ef4da..f5537b2 100644 --- a/middleware/pageprotection.go +++ b/middleware/pageprotection.go @@ -11,6 +11,7 @@ func RequiresLogin(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { user := contexts.GetUser(r.Context()) if user == nil { + w.WriteHeader(http.StatusUnauthorized) page.Error( "401", "Unauthorized", diff --git a/middleware/pageprotection_test.go b/middleware/pageprotection_test.go new file mode 100644 index 0000000..de79975 --- /dev/null +++ b/middleware/pageprotection_test.go @@ -0,0 +1,81 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "projectreshoot/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPageLoginRequired(t *testing.T) { + // Basic setup + cfg, err := tests.TestConfig() + require.NoError(t, err) + logger := tests.NilLogger() + conn, err := tests.SetupTestDB() + require.NoError(t, err) + require.NotNil(t, conn) + defer tests.DeleteTestDB() + + // Handler to check outcome of Authentication middleware + testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + // Add the middleware and create the server + loginRequiredHandler := RequiresLogin(testHandler) + authHandler := Authentication(logger, cfg, conn, loginRequiredHandler) + server := httptest.NewServer(authHandler) + defer server.Close() + + tokens := getTokens() + + tests := []struct { + name string + accessToken string + refreshToken string + expectedCode int + }{ + { + name: "Valid Login", + accessToken: tokens["accessFresh"], + refreshToken: "", + expectedCode: http.StatusOK, + }, + { + name: "Expired login", + accessToken: tokens["accessExpired"], + refreshToken: tokens["refreshExpired"], + expectedCode: http.StatusUnauthorized, + }, + { + name: "No login", + accessToken: "", + refreshToken: "", + expectedCode: http.StatusUnauthorized, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := &http.Client{} + + req, _ := http.NewRequest(http.MethodGet, server.URL, nil) + + // Add cookies if provided + if tt.accessToken != "" { + req.AddCookie(&http.Cookie{Name: "access", Value: tt.accessToken}) + } + if tt.refreshToken != "" { + req.AddCookie(&http.Cookie{Name: "refresh", Value: tt.refreshToken}) + } + + resp, err := client.Do(req) + assert.NoError(t, err) + assert.Equal(t, tt.expectedCode, resp.StatusCode) + }) + } +} diff --git a/middleware/reauthentication_test.go b/middleware/reauthentication_test.go new file mode 100644 index 0000000..63017cb --- /dev/null +++ b/middleware/reauthentication_test.go @@ -0,0 +1,88 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "projectreshoot/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestActionReauthRequired(t *testing.T) { + // Basic setup + cfg, err := tests.TestConfig() + require.NoError(t, err) + logger := tests.NilLogger() + conn, err := tests.SetupTestDB() + require.NoError(t, err) + require.NotNil(t, conn) + defer tests.DeleteTestDB() + + // Handler to check outcome of Authentication middleware + testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + // Add the middleware and create the server + reauthRequiredHandler := RequiresFresh(testHandler) + loginRequiredHandler := RequiresLogin(reauthRequiredHandler) + authHandler := Authentication(logger, cfg, conn, loginRequiredHandler) + server := httptest.NewServer(authHandler) + defer server.Close() + + tokens := getTokens() + + tests := []struct { + name string + accessToken string + refreshToken string + expectedCode int + }{ + { + name: "Fresh Login", + accessToken: tokens["accessFresh"], + refreshToken: "", + expectedCode: http.StatusOK, + }, + { + name: "Unfresh Login", + accessToken: tokens["accessUnfresh"], + refreshToken: "", + expectedCode: 444, + }, + { + name: "Expired login", + accessToken: tokens["accessExpired"], + refreshToken: tokens["refreshExpired"], + expectedCode: http.StatusUnauthorized, + }, + { + name: "No login", + accessToken: "", + refreshToken: "", + expectedCode: http.StatusUnauthorized, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := &http.Client{} + + req, _ := http.NewRequest(http.MethodGet, server.URL, nil) + + // Add cookies if provided + if tt.accessToken != "" { + req.AddCookie(&http.Cookie{Name: "access", Value: tt.accessToken}) + } + if tt.refreshToken != "" { + req.AddCookie(&http.Cookie{Name: "refresh", Value: tt.refreshToken}) + } + + resp, err := client.Do(req) + assert.NoError(t, err) + assert.Equal(t, tt.expectedCode, resp.StatusCode) + }) + } +}