diff --git a/api/user.go b/api/user.go index 98c90b48..a522a27e 100644 --- a/api/user.go +++ b/api/user.go @@ -63,11 +63,6 @@ func (h *handler) createUser(w http.ResponseWriter, r *http.Request) { } func (h *handler) updateUser(w http.ResponseWriter, r *http.Request) { - if !request.IsAdminUser(r) { - json.Forbidden(w, r) - return - } - userID := request.RouteInt64Param(r, "userID") userChanges, err := decodeUserModificationRequest(r.Body) if err != nil { @@ -86,6 +81,18 @@ func (h *handler) updateUser(w http.ResponseWriter, r *http.Request) { return } + if !request.IsAdminUser(r) { + if originalUser.ID != request.UserID(r) { + json.Forbidden(w, r) + return + } + + if userChanges.IsAdmin != nil && *userChanges.IsAdmin { + json.BadRequest(w, r, errors.New("Only administrators can change permissions of standard users")) + return + } + } + userChanges.Update(originalUser) if err := originalUser.ValidateUserModification(); err != nil { json.BadRequest(w, r, err) diff --git a/tests/user_test.go b/tests/user_test.go index bc0b396a..191e2990 100644 --- a/tests/user_test.go +++ b/tests/user_test.go @@ -372,22 +372,56 @@ func TestCannotGetUserAsNonAdmin(t *testing.T) { } func TestCannotUpdateUserAsNonAdmin(t *testing.T) { - username := getRandomUsername() - client := miniflux.New(testBaseURL, testAdminUsername, testAdminPassword) - user, err := client.CreateUser(username, testStandardPassword, false) + adminClient := miniflux.New(testBaseURL, testAdminUsername, testAdminPassword) + + usernameA := getRandomUsername() + userA, err := adminClient.CreateUser(usernameA, testStandardPassword, false) if err != nil { t.Fatal(err) } - client = miniflux.New(testBaseURL, username, testStandardPassword) - _, err = client.UpdateUser(user.ID, &miniflux.UserModification{}) + usernameB := getRandomUsername() + _, err = adminClient.CreateUser(usernameB, testStandardPassword, false) + if err != nil { + t.Fatal(err) + } + + entriesPerPage := 10 + userAClient := miniflux.New(testBaseURL, usernameA, testStandardPassword) + userAAfterUpdate, err := userAClient.UpdateUser(userA.ID, &miniflux.UserModification{EntriesPerPage: &entriesPerPage}) + if err != nil { + t.Fatal(`Standard users should be able to update themselves`) + } + + if userAAfterUpdate.EntriesPerPage != entriesPerPage { + t.Fatalf(`The EntriesPerPage field of this user should be updated`) + } + + isAdmin := true + _, err = userAClient.UpdateUser(userA.ID, &miniflux.UserModification{IsAdmin: &isAdmin}) if err == nil { - t.Fatal(`Standard users should not be able to update any users`) + t.Fatal(`Standard users should not be able to become admin`) + } + + userBClient := miniflux.New(testBaseURL, usernameB, testStandardPassword) + _, err = userBClient.UpdateUser(userA.ID, &miniflux.UserModification{}) + if err == nil { + t.Fatal(`Standard users should not be able to update other users`) } if err != miniflux.ErrForbidden { t.Fatal(`A "Forbidden" error should be raised`) } + + stylesheet := "test" + userC, err := adminClient.UpdateUser(userA.ID, &miniflux.UserModification{Stylesheet: &stylesheet}) + if err != nil { + t.Fatal(`Admin users should be able to update any users`) + } + + if userC.Stylesheet != stylesheet { + t.Fatalf(`The Stylesheet field of this user should be updated`) + } } func TestCannotCreateUserAsNonAdmin(t *testing.T) { diff --git a/ui/settings_update.go b/ui/settings_update.go index 507fd833..468917ef 100644 --- a/ui/settings_update.go +++ b/ui/settings_update.go @@ -22,7 +22,7 @@ func (h *handler) updateSettings(w http.ResponseWriter, r *http.Request) { sess := session.New(h.store, request.SessionID(r)) view := view.New(h.tpl, r, sess) - user, err := h.store.UserByID(request.UserID(r)) + loggedUser, err := h.store.UserByID(request.UserID(r)) if err != nil { html.ServerError(w, r, err) return @@ -41,9 +41,9 @@ func (h *handler) updateSettings(w http.ResponseWriter, r *http.Request) { view.Set("languages", locale.AvailableLanguages()) view.Set("timezones", timezones) view.Set("menu", "settings") - view.Set("user", user) - view.Set("countUnread", h.store.CountUnreadEntries(user.ID)) - view.Set("countErrorFeeds", h.store.CountUserFeedsWithErrors(user.ID)) + view.Set("user", loggedUser) + view.Set("countUnread", h.store.CountUnreadEntries(loggedUser.ID)) + view.Set("countErrorFeeds", h.store.CountUserFeedsWithErrors(loggedUser.ID)) if err := settingsForm.Validate(); err != nil { view.Set("errorMessage", err.Error()) @@ -51,13 +51,13 @@ func (h *handler) updateSettings(w http.ResponseWriter, r *http.Request) { return } - if h.store.AnotherUserExists(user.ID, settingsForm.Username) { + if h.store.AnotherUserExists(loggedUser.ID, settingsForm.Username) { view.Set("errorMessage", "error.user_already_exists") html.OK(w, r, view.Render("settings")) return } - err = h.store.UpdateUser(settingsForm.Merge(user)) + err = h.store.UpdateUser(settingsForm.Merge(loggedUser)) if err != nil { logger.Error("[UI:UpdateSettings] %v", err) view.Set("errorMessage", "error.unable_to_update_user") @@ -65,8 +65,8 @@ func (h *handler) updateSettings(w http.ResponseWriter, r *http.Request) { return } - sess.SetLanguage(user.Language) - sess.SetTheme(user.Theme) + sess.SetLanguage(loggedUser.Language) + sess.SetTheme(loggedUser.Theme) sess.NewFlashMessage(locale.NewPrinter(request.UserLanguage(r)).Printf("alert.prefs_saved")) html.Redirect(w, r, route.Path(h.router, "settings")) }