Changing password should confirm the old password

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 🎅
 

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"}

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
 

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())

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 {

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",

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");

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 (
         <Pref labelId={labelId} title={t("account_basics_password_title")} description={t("account_basics_password_description")}>
             <div aria-labelledby={labelId}>
@@ -119,8 +106,7 @@ const ChangePassword = () => {
             <ChangePasswordDialog
                 key={`changePasswordDialog${dialogKey}`}
                 open={dialogOpen}
-                onCancel={handleDialogCancel}
+                onClose={handleDialogClose}
-                onSubmit={handleDialogSubmit}
             />
         </Pref>
     )
@@ -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 (
         <Dialog open={props.open} onClose={props.onCancel} fullScreen={fullScreen}>
             <DialogTitle>{t("account_basics_password_dialog_title")}</DialogTitle>
             <DialogContent>
+                <TextField
+                    margin="dense"
+                    id="current-password"
+                    label={t("account_basics_password_dialog_current_password_label")}
+                    aria-label={t("account_basics_password_dialog_current_password_label")}
+                    type="password"
+                    value={currentPassword}
+                    onChange={ev => setCurrentPassword(ev.target.value)}
+                    fullWidth
+                    variant="standard"
+                />
                 <TextField
                     margin="dense"
                     id="new-password"
@@ -161,10 +175,15 @@ const ChangePasswordDialog = (props) => {
                     variant="standard"
                 />
             </DialogContent>
-            <DialogActions>
+            <DialogFooter status={errorText}>
-                <Button onClick={props.onCancel}>{t("account_basics_password_dialog_button_cancel")}</Button>
+                <Button onClick={props.onClose}>{t("account_basics_password_dialog_button_cancel")}</Button>
-                <Button onClick={() => props.onSubmit(newPassword)} disabled={!changeButtonEnabled}>{t("account_basics_password_dialog_button_submit")}</Button>
+                <Button
-            </DialogActions>
+                    onClick={handleDialogSubmit}
+                    disabled={newPassword.length === 0 || currentPassword.length === 0 || newPassword !== confirmPassword}
+                >
+                    {t("account_basics_password_dialog_button_submit")}
+                </Button>
+            </DialogFooter>
         </Dialog>
     );
 };

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(
+    const localSubscriptions = (subscriptions?.length > 0)
-        ...subscriptions
+        ? Object.assign(...subscriptions.filter(s => s.baseUrl === config.base_url).map(s => ({[s.topic]: s})))
-            .filter(s => s.baseUrl === config.base_url)
+        : [];
-            .map(s => ({[s.topic]: s}))
-    );
 
     const handleEditClick = (reservation) => {
         setDialogKey(prev => prev+1);