From 8f9dafce2051a14c0daec0bc61a273f46765cda8 Mon Sep 17 00:00:00 2001 From: Hunter Kehoe Date: Sat, 25 Jan 2025 21:25:59 -0700 Subject: [PATCH 1/6] change user password via accounts API --- docs/releases.md | 1 + server/server_admin.go | 6 +++++ server/server_admin_test.go | 46 +++++++++++++++++++++++++++++++++++++ server/types.go | 1 + 4 files changed, 54 insertions(+) diff --git a/docs/releases.md b/docs/releases.md index a76ec6a3..959fbb83 100644 --- a/docs/releases.md +++ b/docs/releases.md @@ -1381,6 +1381,7 @@ and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/release * Write VAPID keys to file in `ntfy webpush --output-file` ([#1138](https://github.com/binwiederhier/ntfy/pull/1138), thanks to [@nogweii](https://github.com/nogweii)) * Add Docker major/minor version to image tags ([#1271](https://github.com/binwiederhier/ntfy/pull/1271), thanks to [@RoboMagus](https://github.com/RoboMagus)) * Add `latest` subscription param for grabbing just the most recent message ([#1216](https://github.com/binwiederhier/ntfy/pull/1216), thanks to [@wunter8](https://github.com/wunter8)) +* You can now change passwords via the accounts API (thanks to [@wunter8](https://github.com/wunter8) for implementing) **Bug fixes + maintenance:** diff --git a/server/server_admin.go b/server/server_admin.go index ac295718..0e7e311e 100644 --- a/server/server_admin.go +++ b/server/server_admin.go @@ -49,6 +49,12 @@ func (s *Server) handleUsersAdd(w http.ResponseWriter, r *http.Request, v *visit if err != nil && !errors.Is(err, user.ErrUserNotFound) { return err } else if u != nil { + if req.Force == true { + if err := s.userManager.ChangePassword(req.Username, req.Password); err != nil { + return err + } + return s.writeJSON(w, newSuccessResponse()) + } return errHTTPConflictUserExists } var tier *user.Tier diff --git a/server/server_admin_test.go b/server/server_admin_test.go index c2f8f95a..70574efe 100644 --- a/server/server_admin_test.go +++ b/server/server_admin_test.go @@ -49,6 +49,52 @@ func TestUser_AddRemove(t *testing.T) { "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 200, rr.Code) + + // Check user was deleted + users, err = s.userManager.Users() + require.Nil(t, err) + require.Equal(t, 3, len(users)) + require.Equal(t, "phil", users[0].Name) + require.Equal(t, "emma", users[1].Name) + require.Equal(t, user.Everyone, users[2].Name) +} + +func TestUser_ChangePassword(t *testing.T) { + s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + + // Create admin + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleAdmin)) + + // Create user via API + rr := request(t, s, "PUT", "/v1/users", `{"username": "ben", "password": "ben"}`, map[string]string{ + "Authorization": util.BasicAuth("phil", "phil"), + }) + require.Equal(t, 200, rr.Code) + + // Try to login with first password + rr = request(t, s, "POST", "/v1/account/token", "", map[string]string{ + "Authorization": util.BasicAuth("ben", "ben"), + }) + require.Equal(t, 200, rr.Code) + + // Change password via API + rr = request(t, s, "PUT", "/v1/users", `{"username": "ben", "password": "ben-two", "force":true}`, map[string]string{ + "Authorization": util.BasicAuth("phil", "phil"), + }) + require.Equal(t, 200, rr.Code) + + // Make sure first password fails + rr = request(t, s, "POST", "/v1/account/token", "", map[string]string{ + "Authorization": util.BasicAuth("ben", "ben"), + }) + require.Equal(t, 401, rr.Code) + + // Try to login with second password + rr = request(t, s, "POST", "/v1/account/token", "", map[string]string{ + "Authorization": util.BasicAuth("ben", "ben-two"), + }) + require.Equal(t, 200, rr.Code) } func TestUser_AddRemove_Failures(t *testing.T) { diff --git a/server/types.go b/server/types.go index c6bdb4d1..1b95a73d 100644 --- a/server/types.go +++ b/server/types.go @@ -257,6 +257,7 @@ type apiUserAddRequest struct { Username string `json:"username"` Password string `json:"password"` Tier string `json:"tier"` + Force bool `json:"force"` // Used to change passwords/override existing user // Do not add 'role' here. We don't want to add admins via the API. } From ad7ab18fb737d22f3cbb434665cd49b0048dba44 Mon Sep 17 00:00:00 2001 From: Hunter Kehoe Date: Sat, 25 Jan 2025 21:37:23 -0700 Subject: [PATCH 2/6] prevent changing admin passwords --- server/server_admin.go | 3 +++ server/server_admin_test.go | 17 ++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/server/server_admin.go b/server/server_admin.go index 0e7e311e..a2654db7 100644 --- a/server/server_admin.go +++ b/server/server_admin.go @@ -50,6 +50,9 @@ func (s *Server) handleUsersAdd(w http.ResponseWriter, r *http.Request, v *visit return err } else if u != nil { if req.Force == true { + if u.IsAdmin() { + return errHTTPForbidden + } if err := s.userManager.ChangePassword(req.Username, req.Password); err != nil { return err } diff --git a/server/server_admin_test.go b/server/server_admin_test.go index 70574efe..c99ec549 100644 --- a/server/server_admin_test.go +++ b/server/server_admin_test.go @@ -59,7 +59,7 @@ func TestUser_AddRemove(t *testing.T) { require.Equal(t, user.Everyone, users[2].Name) } -func TestUser_ChangePassword(t *testing.T) { +func TestUser_ChangeUserPassword(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) defer s.closeDatabases() @@ -97,6 +97,21 @@ func TestUser_ChangePassword(t *testing.T) { require.Equal(t, 200, rr.Code) } +func TestUser_DontChangeAdminPassword(t *testing.T) { + s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + + // Create admin + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleAdmin)) + require.Nil(t, s.userManager.AddUser("admin", "admin", user.RoleAdmin)) + + // Try to change password via API + rr := request(t, s, "PUT", "/v1/users", `{"username": "admin", "password": "admin-new", "force":true}`, map[string]string{ + "Authorization": util.BasicAuth("phil", "phil"), + }) + require.Equal(t, 403, rr.Code) +} + func TestUser_AddRemove_Failures(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) defer s.closeDatabases() From 2b40ad9a129dabbcbe7a90c3be26c9071e50a0ea Mon Sep 17 00:00:00 2001 From: Hunter Kehoe Date: Sat, 25 Jan 2025 21:47:11 -0700 Subject: [PATCH 3/6] make staticcheck happy --- server/server_admin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/server_admin.go b/server/server_admin.go index a2654db7..d1cdefae 100644 --- a/server/server_admin.go +++ b/server/server_admin.go @@ -49,7 +49,7 @@ func (s *Server) handleUsersAdd(w http.ResponseWriter, r *http.Request, v *visit if err != nil && !errors.Is(err, user.ErrUserNotFound) { return err } else if u != nil { - if req.Force == true { + if req.Force { if u.IsAdmin() { return errHTTPForbidden } From fa48639517bd65b4b3f20472a015e6f7af409ea2 Mon Sep 17 00:00:00 2001 From: Hunter Kehoe Date: Thu, 22 May 2025 18:58:37 -0600 Subject: [PATCH 4/6] make POST create user and PUT update user --- server/server.go | 4 ++- server/server_admin.go | 49 +++++++++++++++++++++++++++++-------- server/server_admin_test.go | 14 +++++------ server/types.go | 3 +-- 4 files changed, 50 insertions(+), 20 deletions(-) diff --git a/server/server.go b/server/server.go index c4af5ce8..90e52bf4 100644 --- a/server/server.go +++ b/server/server.go @@ -446,8 +446,10 @@ func (s *Server) handleInternal(w http.ResponseWriter, r *http.Request, v *visit return s.ensureWebPushEnabled(s.handleWebManifest)(w, r, v) } else if r.Method == http.MethodGet && r.URL.Path == apiUsersPath { return s.ensureAdmin(s.handleUsersGet)(w, r, v) - } else if r.Method == http.MethodPut && r.URL.Path == apiUsersPath { + } else if r.Method == http.MethodPost && r.URL.Path == apiUsersPath { return s.ensureAdmin(s.handleUsersAdd)(w, r, v) + } else if r.Method == http.MethodPut && r.URL.Path == apiUsersPath { + return s.ensureAdmin(s.handleUsersUpdate)(w, r, v) } else if r.Method == http.MethodDelete && r.URL.Path == apiUsersPath { return s.ensureAdmin(s.handleUsersDelete)(w, r, v) } else if (r.Method == http.MethodPut || r.Method == http.MethodPost) && r.URL.Path == apiUsersAccessPath { diff --git a/server/server_admin.go b/server/server_admin.go index d1cdefae..e8e91320 100644 --- a/server/server_admin.go +++ b/server/server_admin.go @@ -39,7 +39,7 @@ func (s *Server) handleUsersGet(w http.ResponseWriter, r *http.Request, v *visit } func (s *Server) handleUsersAdd(w http.ResponseWriter, r *http.Request, v *visitor) error { - req, err := readJSONWithLimit[apiUserAddRequest](r.Body, jsonBodyBytesLimit, false) + req, err := readJSONWithLimit[apiUserAddOrUpdateRequest](r.Body, jsonBodyBytesLimit, false) if err != nil { return err } else if !user.AllowedUsername(req.Username) || req.Password == "" { @@ -49,15 +49,6 @@ func (s *Server) handleUsersAdd(w http.ResponseWriter, r *http.Request, v *visit if err != nil && !errors.Is(err, user.ErrUserNotFound) { return err } else if u != nil { - if req.Force { - if u.IsAdmin() { - return errHTTPForbidden - } - if err := s.userManager.ChangePassword(req.Username, req.Password); err != nil { - return err - } - return s.writeJSON(w, newSuccessResponse()) - } return errHTTPConflictUserExists } var tier *user.Tier @@ -79,6 +70,44 @@ func (s *Server) handleUsersAdd(w http.ResponseWriter, r *http.Request, v *visit } return s.writeJSON(w, newSuccessResponse()) } +func (s *Server) handleUsersUpdate(w http.ResponseWriter, r *http.Request, v *visitor) error { + req, err := readJSONWithLimit[apiUserAddOrUpdateRequest](r.Body, jsonBodyBytesLimit, false) + if err != nil { + return err + } else if !user.AllowedUsername(req.Username) || req.Password == "" { + return errHTTPBadRequest.Wrap("username invalid, or password missing") + } + u, err := s.userManager.User(req.Username) + if err != nil && !errors.Is(err, user.ErrUserNotFound) { + return err + } else if u != nil { + if u.IsAdmin() { + return errHTTPForbidden + } + if err := s.userManager.ChangePassword(req.Username, req.Password); err != nil { + return err + } + return s.writeJSON(w, newSuccessResponse()) + } + var tier *user.Tier + if req.Tier != "" { + tier, err = s.userManager.Tier(req.Tier) + if errors.Is(err, user.ErrTierNotFound) { + return errHTTPBadRequestTierInvalid + } else if err != nil { + return err + } + } + if err := s.userManager.AddUser(req.Username, req.Password, user.RoleUser); err != nil { + return err + } + if tier != nil { + if err := s.userManager.ChangeTier(req.Username, req.Tier); err != nil { + return err + } + } + return s.writeJSON(w, newSuccessResponse()) +} func (s *Server) handleUsersDelete(w http.ResponseWriter, r *http.Request, v *visitor) error { req, err := readJSONWithLimit[apiUserDeleteRequest](r.Body, jsonBodyBytesLimit, false) diff --git a/server/server_admin_test.go b/server/server_admin_test.go index c99ec549..194b0a18 100644 --- a/server/server_admin_test.go +++ b/server/server_admin_test.go @@ -20,7 +20,7 @@ func TestUser_AddRemove(t *testing.T) { })) // Create user via API - rr := request(t, s, "PUT", "/v1/users", `{"username": "ben", "password":"ben"}`, map[string]string{ + rr := request(t, s, "POST", "/v1/users", `{"username": "ben", "password":"ben"}`, map[string]string{ "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 200, rr.Code) @@ -67,7 +67,7 @@ func TestUser_ChangeUserPassword(t *testing.T) { require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleAdmin)) // Create user via API - rr := request(t, s, "PUT", "/v1/users", `{"username": "ben", "password": "ben"}`, map[string]string{ + rr := request(t, s, "POST", "/v1/users", `{"username": "ben", "password": "ben"}`, map[string]string{ "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 200, rr.Code) @@ -79,7 +79,7 @@ func TestUser_ChangeUserPassword(t *testing.T) { require.Equal(t, 200, rr.Code) // Change password via API - rr = request(t, s, "PUT", "/v1/users", `{"username": "ben", "password": "ben-two", "force":true}`, map[string]string{ + rr = request(t, s, "PUT", "/v1/users", `{"username": "ben", "password": "ben-two"}`, map[string]string{ "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 200, rr.Code) @@ -106,7 +106,7 @@ func TestUser_DontChangeAdminPassword(t *testing.T) { require.Nil(t, s.userManager.AddUser("admin", "admin", user.RoleAdmin)) // Try to change password via API - rr := request(t, s, "PUT", "/v1/users", `{"username": "admin", "password": "admin-new", "force":true}`, map[string]string{ + rr := request(t, s, "PUT", "/v1/users", `{"username": "admin", "password": "admin-new"}`, map[string]string{ "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 403, rr.Code) @@ -121,19 +121,19 @@ func TestUser_AddRemove_Failures(t *testing.T) { require.Nil(t, s.userManager.AddUser("ben", "ben", user.RoleUser)) // Cannot create user with invalid username - rr := request(t, s, "PUT", "/v1/users", `{"username": "not valid", "password":"ben"}`, map[string]string{ + rr := request(t, s, "POST", "/v1/users", `{"username": "not valid", "password":"ben"}`, map[string]string{ "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 400, rr.Code) // Cannot create user if user already exists - rr = request(t, s, "PUT", "/v1/users", `{"username": "phil", "password":"phil"}`, map[string]string{ + rr = request(t, s, "POST", "/v1/users", `{"username": "phil", "password":"phil"}`, map[string]string{ "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 40901, toHTTPError(t, rr.Body.String()).Code) // Cannot create user with invalid tier - rr = request(t, s, "PUT", "/v1/users", `{"username": "emma", "password":"emma", "tier": "invalid"}`, map[string]string{ + rr = request(t, s, "POST", "/v1/users", `{"username": "emma", "password":"emma", "tier": "invalid"}`, map[string]string{ "Authorization": util.BasicAuth("phil", "phil"), }) require.Equal(t, 40030, toHTTPError(t, rr.Body.String()).Code) diff --git a/server/types.go b/server/types.go index 1b95a73d..fd1931f5 100644 --- a/server/types.go +++ b/server/types.go @@ -253,11 +253,10 @@ type apiStatsResponse struct { MessagesRate float64 `json:"messages_rate"` // Average number of messages per second } -type apiUserAddRequest struct { +type apiUserAddOrUpdateRequest struct { Username string `json:"username"` Password string `json:"password"` Tier string `json:"tier"` - Force bool `json:"force"` // Used to change passwords/override existing user // Do not add 'role' here. We don't want to add admins via the API. } From e36e4856c9d04b2ad0e8ad5d080809d45c7b6cd0 Mon Sep 17 00:00:00 2001 From: Hunter Kehoe Date: Thu, 22 May 2025 19:57:57 -0600 Subject: [PATCH 5/6] allow changing password or tier with user PUT --- server/server_admin.go | 18 +++++++++------ server/server_admin_test.go | 46 +++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/server/server_admin.go b/server/server_admin.go index e8e91320..7c78adb3 100644 --- a/server/server_admin.go +++ b/server/server_admin.go @@ -74,8 +74,10 @@ func (s *Server) handleUsersUpdate(w http.ResponseWriter, r *http.Request, v *vi req, err := readJSONWithLimit[apiUserAddOrUpdateRequest](r.Body, jsonBodyBytesLimit, false) if err != nil { return err - } else if !user.AllowedUsername(req.Username) || req.Password == "" { - return errHTTPBadRequest.Wrap("username invalid, or password missing") + } else if !user.AllowedUsername(req.Username) { + return errHTTPBadRequest.Wrap("username invalid") + } else if req.Password == "" && req.Tier == "" { + return errHTTPBadRequest.Wrap("need to provide at least one of \"password\" or \"tier\"") } u, err := s.userManager.User(req.Username) if err != nil && !errors.Is(err, user.ErrUserNotFound) { @@ -84,10 +86,15 @@ func (s *Server) handleUsersUpdate(w http.ResponseWriter, r *http.Request, v *vi if u.IsAdmin() { return errHTTPForbidden } - if err := s.userManager.ChangePassword(req.Username, req.Password); err != nil { + if req.Password != "" { + if err := s.userManager.ChangePassword(req.Username, req.Password); err != nil { + return err + } + } + } else { + if err := s.userManager.AddUser(req.Username, req.Password, user.RoleUser); err != nil { return err } - return s.writeJSON(w, newSuccessResponse()) } var tier *user.Tier if req.Tier != "" { @@ -98,9 +105,6 @@ func (s *Server) handleUsersUpdate(w http.ResponseWriter, r *http.Request, v *vi return err } } - if err := s.userManager.AddUser(req.Username, req.Password, user.RoleUser); err != nil { - return err - } if tier != nil { if err := s.userManager.ChangeTier(req.Username, req.Tier); err != nil { return err diff --git a/server/server_admin_test.go b/server/server_admin_test.go index 194b0a18..80da3224 100644 --- a/server/server_admin_test.go +++ b/server/server_admin_test.go @@ -57,6 +57,12 @@ func TestUser_AddRemove(t *testing.T) { require.Equal(t, "phil", users[0].Name) require.Equal(t, "emma", users[1].Name) require.Equal(t, user.Everyone, users[2].Name) + + // Reject invalid user change + rr = request(t, s, "PUT", "/v1/users", `{"username": "ben"}`, map[string]string{ + "Authorization": util.BasicAuth("phil", "phil"), + }) + require.Equal(t, 400, rr.Code) } func TestUser_ChangeUserPassword(t *testing.T) { @@ -97,6 +103,46 @@ func TestUser_ChangeUserPassword(t *testing.T) { require.Equal(t, 200, rr.Code) } +func TestUser_ChangeUserTier(t *testing.T) { + s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + + // Create admin, tier + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleAdmin)) + require.Nil(t, s.userManager.AddTier(&user.Tier{ + Code: "tier1", + })) + require.Nil(t, s.userManager.AddTier(&user.Tier{ + Code: "tier2", + })) + + // Create user with tier via API + rr := request(t, s, "POST", "/v1/users", `{"username": "ben", "password":"ben", "tier": "tier1"}`, map[string]string{ + "Authorization": util.BasicAuth("phil", "phil"), + }) + require.Equal(t, 200, rr.Code) + + // Check users + users, err := s.userManager.Users() + require.Nil(t, err) + require.Equal(t, 3, len(users)) + require.Equal(t, "phil", users[0].Name) + require.Equal(t, "ben", users[1].Name) + require.Equal(t, user.RoleUser, users[1].Role) + require.Equal(t, "tier1", users[1].Tier.Code) + + // Change user tier via API + rr = request(t, s, "PUT", "/v1/users", `{"username": "ben", "tier": "tier2"}`, map[string]string{ + "Authorization": util.BasicAuth("phil", "phil"), + }) + require.Equal(t, 200, rr.Code) + + // Check users again + users, err = s.userManager.Users() + require.Nil(t, err) + require.Equal(t, "tier2", users[1].Tier.Code) +} + func TestUser_DontChangeAdminPassword(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) defer s.closeDatabases() From 0fb60ae72d8387abf6280874d85379cb502d6bca Mon Sep 17 00:00:00 2001 From: Hunter Kehoe Date: Thu, 22 May 2025 20:01:50 -0600 Subject: [PATCH 6/6] test change user password and tier in single request --- server/server_admin_test.go | 52 +++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/server/server_admin_test.go b/server/server_admin_test.go index 80da3224..98c268e9 100644 --- a/server/server_admin_test.go +++ b/server/server_admin_test.go @@ -143,6 +143,58 @@ func TestUser_ChangeUserTier(t *testing.T) { require.Equal(t, "tier2", users[1].Tier.Code) } +func TestUser_ChangeUserPasswordAndTier(t *testing.T) { + s := newTestServer(t, newTestConfigWithAuthFile(t)) + defer s.closeDatabases() + + // Create admin, tier + require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleAdmin)) + require.Nil(t, s.userManager.AddTier(&user.Tier{ + Code: "tier1", + })) + require.Nil(t, s.userManager.AddTier(&user.Tier{ + Code: "tier2", + })) + + // Create user with tier via API + rr := request(t, s, "POST", "/v1/users", `{"username": "ben", "password":"ben", "tier": "tier1"}`, map[string]string{ + "Authorization": util.BasicAuth("phil", "phil"), + }) + require.Equal(t, 200, rr.Code) + + // Check users + users, err := s.userManager.Users() + require.Nil(t, err) + require.Equal(t, 3, len(users)) + require.Equal(t, "phil", users[0].Name) + require.Equal(t, "ben", users[1].Name) + require.Equal(t, user.RoleUser, users[1].Role) + require.Equal(t, "tier1", users[1].Tier.Code) + + // Change user password and tier via API + rr = request(t, s, "PUT", "/v1/users", `{"username": "ben", "password":"ben-two", "tier": "tier2"}`, map[string]string{ + "Authorization": util.BasicAuth("phil", "phil"), + }) + require.Equal(t, 200, rr.Code) + + // Make sure first password fails + rr = request(t, s, "POST", "/v1/account/token", "", map[string]string{ + "Authorization": util.BasicAuth("ben", "ben"), + }) + require.Equal(t, 401, rr.Code) + + // Try to login with second password + rr = request(t, s, "POST", "/v1/account/token", "", map[string]string{ + "Authorization": util.BasicAuth("ben", "ben-two"), + }) + require.Equal(t, 200, rr.Code) + + // Check new tier + users, err = s.userManager.Users() + require.Nil(t, err) + require.Equal(t, "tier2", users[1].Tier.Code) +} + func TestUser_DontChangeAdminPassword(t *testing.T) { s := newTestServer(t, newTestConfigWithAuthFile(t)) defer s.closeDatabases()