From 29f52515e1cd0bc1afc211257e2b28dd8cc60a50 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 31 Aug 2022 12:46:03 +0200 Subject: [PATCH] graph: Fix Status code when updating the password Up to now the /me/changePassword endpoint return a 500 Status when issue a password change with the old password set to the wrong password. This changes the code to return 400 (Bad Request) with an additional message that the old password is wrong. This does not seem to weaken the security of /me/changePassword (i.e. for allowing easier brute-force attacks) as the endpoint is only available to already authenticated users (and only for changing their own passwords) See #4480 --- services/graph/pkg/service/v0/password.go | 8 +++++++- services/graph/pkg/service/v0/password_test.go | 2 +- .../features/apiGraph/changeOwnPassword.feature | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/services/graph/pkg/service/v0/password.go b/services/graph/pkg/service/v0/password.go index b5318953d..4804eb07c 100644 --- a/services/graph/pkg/service/v0/password.go +++ b/services/graph/pkg/service/v0/password.go @@ -69,7 +69,13 @@ func (g Graph) ChangeOwnPassword(w http.ResponseWriter, r *http.Request) { return } - if authRes.Status.Code != cs3rpc.Code_CODE_OK { + switch authRes.Status.Code { + case cs3rpc.Code_CODE_OK: + break + case cs3rpc.Code_CODE_UNAUTHENTICATED, cs3rpc.Code_CODE_PERMISSION_DENIED: + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "wrong current password") + return + default: errorcode.InvalidRequest.Render(w, r, http.StatusInternalServerError, "password change failed") return } diff --git a/services/graph/pkg/service/v0/password_test.go b/services/graph/pkg/service/v0/password_test.go index a31f7775e..84a33fd07 100644 --- a/services/graph/pkg/service/v0/password_test.go +++ b/services/graph/pkg/service/v0/password_test.go @@ -122,7 +122,7 @@ var _ = Describe("Users changing their own password", func() { Entry("fails when new password is empty", "currentpassword", "", "", http.StatusBadRequest), Entry("fails when current and new password are equal", "password", "password", "", http.StatusBadRequest), Entry("fails authentication with current password errors", "currentpassword", "newpassword", "error", http.StatusInternalServerError), - Entry("fails when current password is wrong", "currentpassword", "newpassword", "deny", http.StatusInternalServerError), + Entry("fails when current password is wrong", "currentpassword", "newpassword", "deny", http.StatusBadRequest), Entry("succeeds when current password is correct", "currentpassword", "newpassword", "", http.StatusNoContent), ) }) diff --git a/tests/acceptance/features/apiGraph/changeOwnPassword.feature b/tests/acceptance/features/apiGraph/changeOwnPassword.feature index a0351cc8d..e487455a8 100644 --- a/tests/acceptance/features/apiGraph/changeOwnPassword.feature +++ b/tests/acceptance/features/apiGraph/changeOwnPassword.feature @@ -13,5 +13,5 @@ Feature: an user changes its own password | 123456 | ?&^%0 | 204 | | 123456 | | 400 | | 123456 | 123456 | 400 | - | wrongPass | 123456 | 500 | + | wrongPass | 123456 | 400 | | | validPass | 400 |