From 88abd8872d7c1c8b3323168a48d2550251558e6f Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Sat, 21 Jan 2023 20:52:16 -0500 Subject: [PATCH] Changing password should confirm the old password --- docs/releases.md | 5 +++ server/errors.go | 2 + server/server.go | 4 +- server/server_account.go | 9 +++- server/types.go | 3 +- web/public/static/langs/en.json | 2 + web/src/app/AccountApi.js | 15 +++++-- web/src/components/Account.js | 69 ++++++++++++++++++++----------- web/src/components/Preferences.js | 8 ++-- 9 files changed, 78 insertions(+), 39 deletions(-) diff --git a/docs/releases.md b/docs/releases.md index f5fa37b4..dc4872ea 100644 --- a/docs/releases.md +++ b/docs/releases.md @@ -24,6 +24,11 @@ and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/release * Portuguese (thanks to [@ssantos](https://hosted.weblate.org/user/ssantos/)) +**Special thanks:** + +A big Thank-you goes to everyone who tested the user account and payments work. I very much appreciate all the feedback, +suggestions, and bug reports. Thank you, @nwithan8, @deadcade, and @xenrox. + ## ntfy server v1.30.1 Released December 23, 2022 🎅 diff --git a/server/errors.go b/server/errors.go index 20bf2e84..be63a54e 100644 --- a/server/errors.go +++ b/server/errors.go @@ -33,6 +33,7 @@ func wrapErrHTTP(err *errHTTP, message string, args ...any) *errHTTP { } var ( + errHTTPBadRequest = &errHTTP{40000, http.StatusBadRequest, "invalid request", ""} errHTTPBadRequestEmailDisabled = &errHTTP{40001, http.StatusBadRequest, "e-mail notifications are not enabled", "https://ntfy.sh/docs/config/#e-mail-notifications"} errHTTPBadRequestDelayNoCache = &errHTTP{40002, http.StatusBadRequest, "cannot disable cache for delayed message", ""} errHTTPBadRequestDelayNoEmail = &errHTTP{40003, http.StatusBadRequest, "delayed e-mail notifications are not supported", ""} @@ -61,6 +62,7 @@ var ( errHTTPBadRequestNotAPaidUser = &errHTTP{40027, http.StatusBadRequest, "invalid request: not a paid user", ""} errHTTPBadRequestBillingRequestInvalid = &errHTTP{40028, http.StatusBadRequest, "invalid request: not a valid billing request", ""} errHTTPBadRequestBillingSubscriptionExists = &errHTTP{40029, http.StatusBadRequest, "invalid request: billing subscription already exists", ""} + errHTTPBadRequestCurrentPasswordWrong = &errHTTP{40030, http.StatusBadRequest, "invalid request: current password is not correct", ""} errHTTPNotFound = &errHTTP{40401, http.StatusNotFound, "page not found", ""} errHTTPUnauthorized = &errHTTP{40101, http.StatusUnauthorized, "unauthorized", "https://ntfy.sh/docs/publish/#authentication"} errHTTPForbidden = &errHTTP{40301, http.StatusForbidden, "forbidden", "https://ntfy.sh/docs/publish/#authentication"} diff --git a/server/server.go b/server/server.go index 42216531..12cff2b1 100644 --- a/server/server.go +++ b/server/server.go @@ -38,13 +38,12 @@ import ( TODO -- -UAT results (round 1): - Security: Account re-creation leads to terrible behavior. Use user ID instead of user name for (a) visitor map, (b) messages.user column, (c) Stripe checkout session -- Account: Changing password should confirm the old password (Thorben) - Reservation: Kill existing subscribers when topic is reserved (deadcade) - Reservation (UI): Show "This topic is reserved" error message when trying to reserve a reserved topic (Thorben) - Reservation (UI): Ask for confirmation when removing reservation (deadcade) - Logging: Add detailed logging with username/customerID for all Stripe events (phil) +- Rate limiting: Sensitive endpoints (account/login/change-password/...) races: - v.user --> see publishSyncEventAsync() test @@ -59,7 +58,6 @@ Limits & rate limiting: rate limiting weirdness. wth is going on? bandwidth limit must be in tier users without tier: should the stats be persisted? are they meaningful? -> test that the visitor is based on the IP address! - login/account endpoints when ResetStats() is run, reset messagesLimiter (and others)? Delete visitor when tier is changed to refresh rate limiters diff --git a/server/server_account.go b/server/server_account.go index 6bcb3233..755dcf75 100644 --- a/server/server_account.go +++ b/server/server_account.go @@ -136,11 +136,16 @@ func (s *Server) handleAccountDelete(w http.ResponseWriter, _ *http.Request, v * } func (s *Server) handleAccountPasswordChange(w http.ResponseWriter, r *http.Request, v *visitor) error { - newPassword, err := readJSONWithLimit[apiAccountPasswordChangeRequest](r.Body, jsonBodyBytesLimit) + req, err := readJSONWithLimit[apiAccountPasswordChangeRequest](r.Body, jsonBodyBytesLimit) if err != nil { return err + } else if req.Password == "" || req.NewPassword == "" { + return errHTTPBadRequest } - if err := s.userManager.ChangePassword(v.user.Name, newPassword.Password); err != nil { + if _, err := s.userManager.Authenticate(v.user.Name, req.Password); err != nil { + return errHTTPBadRequestCurrentPasswordWrong + } + if err := s.userManager.ChangePassword(v.user.Name, req.NewPassword); err != nil { return err } return s.writeJSON(w, newSuccessResponse()) diff --git a/server/types.go b/server/types.go index 5d0f04b5..3d651d30 100644 --- a/server/types.go +++ b/server/types.go @@ -227,7 +227,8 @@ type apiAccountCreateRequest struct { } type apiAccountPasswordChangeRequest struct { - Password string `json:"password"` + Password string `json:"password"` + NewPassword string `json:"new_password"` } type apiAccountTokenResponse struct { diff --git a/web/public/static/langs/en.json b/web/public/static/langs/en.json index cac86d87..33442e44 100644 --- a/web/public/static/langs/en.json +++ b/web/public/static/langs/en.json @@ -170,10 +170,12 @@ "account_basics_password_title": "Password", "account_basics_password_description": "Change your account password", "account_basics_password_dialog_title": "Change password", + "account_basics_password_dialog_current_password_label": "Current password", "account_basics_password_dialog_new_password_label": "New password", "account_basics_password_dialog_confirm_password_label": "Confirm password", "account_basics_password_dialog_button_cancel": "Cancel", "account_basics_password_dialog_button_submit": "Change password", + "account_basics_password_dialog_current_password_incorrect": "Current password incorrect", "account_usage_title": "Usage", "account_usage_of_limit": "of {{limit}}", "account_usage_unlimited": "Unlimited", diff --git a/web/src/app/AccountApi.js b/web/src/app/AccountApi.js index 05b3d6b6..86670f02 100644 --- a/web/src/app/AccountApi.js +++ b/web/src/app/AccountApi.js @@ -120,17 +120,20 @@ class AccountApi { } } - async changePassword(newPassword) { + async changePassword(currentPassword, newPassword) { const url = accountPasswordUrl(config.base_url); console.log(`[AccountApi] Changing account password ${url}`); const response = await fetch(url, { method: "POST", headers: withBearerAuth({}, session.token()), body: JSON.stringify({ - password: newPassword + password: currentPassword, + new_password: newPassword }) }); - if (response.status === 401 || response.status === 403) { + if (response.status === 400) { + throw new CurrentPasswordWrongError(); + } else if (response.status === 401 || response.status === 403) { throw new UnauthorizedError(); } else if (response.status !== 200) { throw new Error(`Unexpected server response ${response.status}`); @@ -394,6 +397,12 @@ export class AccountCreateLimitReachedError extends Error { } } +export class CurrentPasswordWrongError extends Error { + constructor() { + super("Current password incorrect"); + } +} + export class UnauthorizedError extends Error { constructor() { super("Unauthorized"); diff --git a/web/src/components/Account.js b/web/src/components/Account.js index 452868b7..67c2d4f0 100644 --- a/web/src/components/Account.js +++ b/web/src/components/Account.js @@ -19,7 +19,7 @@ import DialogActions from "@mui/material/DialogActions"; import routes from "./routes"; import IconButton from "@mui/material/IconButton"; import {formatBytes, formatShortDate, formatShortDateTime} from "../app/utils"; -import accountApi, {UnauthorizedError} from "../app/AccountApi"; +import accountApi, {CurrentPasswordWrongError, UnauthorizedError} from "../app/AccountApi"; import InfoOutlinedIcon from '@mui/icons-material/InfoOutlined'; import {Pref, PrefGroup} from "./Pref"; import db from "../app/db"; @@ -29,6 +29,7 @@ import UpgradeDialog from "./UpgradeDialog"; import CelebrationIcon from "@mui/icons-material/Celebration"; import {AccountContext} from "./App"; import {Warning, WarningAmber} from "@mui/icons-material"; +import DialogFooter from "./DialogFooter"; const Account = () => { if (!session.exists()) { @@ -90,24 +91,10 @@ const ChangePassword = () => { setDialogOpen(true); }; - const handleDialogCancel = () => { + const handleDialogClose = () => { setDialogOpen(false); }; - const handleDialogSubmit = async (newPassword) => { - try { - await accountApi.changePassword(newPassword); - setDialogOpen(false); - console.debug(`[Account] Password changed`); - } catch (e) { - console.log(`[Account] Error changing password`, e); - if ((e instanceof UnauthorizedError)) { - session.resetAndRedirect(routes.login); - } - // TODO show error - } - }; - return (
@@ -119,8 +106,7 @@ const ChangePassword = () => { ) @@ -128,16 +114,44 @@ const ChangePassword = () => { const ChangePasswordDialog = (props) => { const { t } = useTranslation(); + const [currentPassword, setCurrentPassword] = useState(""); const [newPassword, setNewPassword] = useState(""); const [confirmPassword, setConfirmPassword] = useState(""); + const [errorText, setErrorText] = useState(""); + const fullScreen = useMediaQuery(theme.breakpoints.down('sm')); - const changeButtonEnabled = (() => { - return newPassword.length > 0 && newPassword === confirmPassword; - })(); + + const handleDialogSubmit = async () => { + try { + console.debug(`[Account] Changing password`); + await accountApi.changePassword(currentPassword, newPassword); + props.onClose(); + } catch (e) { + console.log(`[Account] Error changing password`, e); + if ((e instanceof CurrentPasswordWrongError)) { + setErrorText(t("account_basics_password_dialog_current_password_incorrect")); + } else if ((e instanceof UnauthorizedError)) { + session.resetAndRedirect(routes.login); + } + // TODO show error + } + }; + return ( {t("account_basics_password_dialog_title")} + setCurrentPassword(ev.target.value)} + fullWidth + variant="standard" + /> { variant="standard" /> - - - - + + + + ); }; diff --git a/web/src/components/Preferences.js b/web/src/components/Preferences.js index 88b97fc2..2ec59738 100644 --- a/web/src/components/Preferences.js +++ b/web/src/components/Preferences.js @@ -548,11 +548,9 @@ const ReservationsTable = (props) => { const [dialogOpen, setDialogOpen] = useState(false); const [dialogReservation, setDialogReservation] = useState(null); const { subscriptions } = useOutletContext(); - const localSubscriptions = Object.assign( - ...subscriptions - .filter(s => s.baseUrl === config.base_url) - .map(s => ({[s.topic]: s})) - ); + const localSubscriptions = (subscriptions?.length > 0) + ? Object.assign(...subscriptions.filter(s => s.baseUrl === config.base_url).map(s => ({[s.topic]: s}))) + : []; const handleEditClick = (reservation) => { setDialogKey(prev => prev+1);