JS error handling

server/errors.go

@@ -58,11 +58,10 @@ var (
 	errHTTPBadRequestNoTokenProvided                 = &errHTTP{40023, http.StatusBadRequest, "invalid request: no token provided", ""}
 	errHTTPBadRequestJSONInvalid                     = &errHTTP{40024, http.StatusBadRequest, "invalid request: request body must be valid JSON", ""}
 	errHTTPBadRequestPermissionInvalid               = &errHTTP{40025, http.StatusBadRequest, "invalid request: incorrect permission string", ""}
-	errHTTPBadRequestMakesNoSenseForAdmin            = &errHTTP{40026, http.StatusBadRequest, "invalid request: this makes no sense for admins", ""}
+	errHTTPBadRequestIncorrectPasswordConfirmation   = &errHTTP{40026, http.StatusBadRequest, "invalid request: password confirmation is not correct", ""}
 	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", ""}
-	errHTTPBadRequestIncorrectPasswordConfirmation   = &errHTTP{40030, http.StatusBadRequest, "invalid request: password confirmation 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

@@ -35,11 +35,14 @@ import (
 /*
 
 - HIGH Rate limiting: Sensitive endpoints (account/login/change-password/...)
+- HIGH Account limit creation triggers when account is taken!
 - HIGH Docs
-- HIGH CLI
+- HIGH CLI "ntfy tier [add|list|delete]"
+- HIGH CLI "ntfy user" should show tier
+- HIGH Self-review
+- HIGH Stripe webhook failures cannot be diagnosed because of missing logs
 - MEDIUM: Test for expiring messages after reservation removal
 - MEDIUM: Test new token endpoints & never-expiring token
-- MEDIUM: Make sure account endpoints make sense for admins
 - LOW: UI: Flickering upgrade banner when logging in
 
 */

server/server_account.go

@@ -20,8 +20,7 @@ const (
 
 func (s *Server) handleAccountCreate(w http.ResponseWriter, r *http.Request, v *visitor) error {
 	u := v.User()
-	admin := u != nil && u.Role == user.RoleAdmin
-	if !admin {
+	if !u.Admin() { // u may be nil, but that's fine
 		if !s.config.EnableSignup {
 			return errHTTPBadRequestSignupNotEnabled
 		} else if u != nil {
@@ -380,11 +379,11 @@ func (s *Server) handleAccountSubscriptionDelete(w http.ResponseWriter, r *http.
 	return s.writeJSON(w, newSuccessResponse())
 }
 
+// handleAccountReservationAdd adds a topic reservation for the logged-in user, but only if the user has a tier
+// with enough remaining reservations left, or if the user is an admin. Admins can always reserve a topic, unless
+// it is already reserved by someone else.
 func (s *Server) handleAccountReservationAdd(w http.ResponseWriter, r *http.Request, v *visitor) error {
 	u := v.User()
-	if u != nil && u.Role == user.RoleAdmin {
-		return errHTTPBadRequestMakesNoSenseForAdmin
-	}
 	req, err := readJSONWithLimit[apiAccountReservationRequest](r.Body, jsonBodyBytesLimit, false)
 	if err != nil {
 		return err
@@ -396,23 +395,23 @@ func (s *Server) handleAccountReservationAdd(w http.ResponseWriter, r *http.Requ
 	if err != nil {
 		return errHTTPBadRequestPermissionInvalid
 	}
-	if u.Tier == nil {
+	// Check if we are allowed to reserve this topic
+	if u.User() && u.Tier == nil {
 		return errHTTPUnauthorized
-	}
-	// CHeck if we are allowed to reserve this topic
-	if err := s.userManager.CheckAllowAccess(u.Name, req.Topic); err != nil {
+	} else if err := s.userManager.CheckAllowAccess(u.Name, req.Topic); err != nil {
 		return errHTTPConflictTopicReserved
-	}
-	hasReservation, err := s.userManager.HasReservation(u.Name, req.Topic)
-	if err != nil {
-		return err
-	}
-	if !hasReservation {
-		reservations, err := s.userManager.ReservationsCount(u.Name)
+	} else if u.User() {
+		hasReservation, err := s.userManager.HasReservation(u.Name, req.Topic)
 		if err != nil {
 			return err
-		} else if reservations >= u.Tier.ReservationLimit {
-			return errHTTPTooManyRequestsLimitReservations
+		}
+		if !hasReservation {
+			reservations, err := s.userManager.ReservationsCount(u.Name)
+			if err != nil {
+				return err
+			} else if reservations >= u.Tier.ReservationLimit {
+				return errHTTPTooManyRequestsLimitReservations
+			}
 		}
 	}
 	// Actually add the reservation
@@ -428,6 +427,7 @@ func (s *Server) handleAccountReservationAdd(w http.ResponseWriter, r *http.Requ
 	return s.writeJSON(w, newSuccessResponse())
 }
 
+// handleAccountReservationDelete deletes a topic reservation if it is owned by the current user
 func (s *Server) handleAccountReservationDelete(w http.ResponseWriter, r *http.Request, v *visitor) error {
 	matches := apiAccountReservationSingleRegex.FindStringSubmatch(r.URL.Path)
 	if len(matches) != 2 {

server/server_account_test.go

@@ -435,13 +435,52 @@ func TestAccount_Reservation_AddAdminSuccess(t *testing.T) {
 	conf := newTestConfigWithAuthFile(t)
 	conf.EnableSignup = true
 	s := newTestServer(t, conf)
+
+	// A user, an admin, and a reservation walk into a bar
+	require.Nil(t, s.userManager.CreateTier(&user.Tier{
+		Code:             "pro",
+		ReservationLimit: 2,
+	}))
+	require.Nil(t, s.userManager.AddUser("noadmin1", "pass", user.RoleUser))
+	require.Nil(t, s.userManager.ChangeTier("noadmin1", "pro"))
+	require.Nil(t, s.userManager.AddReservation("noadmin1", "mytopic", user.PermissionDenyAll))
+
+	require.Nil(t, s.userManager.AddUser("noadmin2", "pass", user.RoleUser))
+	require.Nil(t, s.userManager.ChangeTier("noadmin2", "pro"))
+
 	require.Nil(t, s.userManager.AddUser("phil", "adminpass", user.RoleAdmin))
 
-	rr := request(t, s, "POST", "/v1/account/reservation", `{"topic":"mytopic","everyone":"deny-all"}`, map[string]string{
+	// Admin can reserve topic
+	rr := request(t, s, "POST", "/v1/account/reservation", `{"topic":"sometopic","everyone":"deny-all"}`, map[string]string{
 		"Authorization": util.BasicAuth("phil", "adminpass"),
 	})
-	require.Equal(t, 400, rr.Code)
-	require.Equal(t, 40026, toHTTPError(t, rr.Body.String()).Code)
+	require.Equal(t, 200, rr.Code)
+
+	// User cannot reserve already reserved topic
+	rr = request(t, s, "POST", "/v1/account/reservation", `{"topic":"mytopic","everyone":"deny-all"}`, map[string]string{
+		"Authorization": util.BasicAuth("noadmin2", "pass"),
+	})
+	require.Equal(t, 409, rr.Code)
+
+	// Admin cannot reserve already reserved topic
+	rr = request(t, s, "POST", "/v1/account/reservation", `{"topic":"mytopic","everyone":"deny-all"}`, map[string]string{
+		"Authorization": util.BasicAuth("phil", "adminpass"),
+	})
+	require.Equal(t, 409, rr.Code)
+
+	reservations, err := s.userManager.Reservations("phil")
+	require.Nil(t, err)
+	require.Equal(t, 1, len(reservations))
+	require.Equal(t, "sometopic", reservations[0].Topic)
+
+	reservations, err = s.userManager.Reservations("noadmin1")
+	require.Nil(t, err)
+	require.Equal(t, 1, len(reservations))
+	require.Equal(t, "mytopic", reservations[0].Topic)
+
+	reservations, err = s.userManager.Reservations("noadmin2")
+	require.Nil(t, err)
+	require.Equal(t, 0, len(reservations))
 }
 
 func TestAccount_Reservation_AddRemoveUserWithTierSuccess(t *testing.T) {

server/visitor.go

@@ -254,6 +254,12 @@ func (v *visitor) User() *user.User {
 	return v.user // May be nil
 }
 
+// Admin returns true if the visitor is a user, and an admin
+func (v *visitor) Admin() bool {
+	u := v.User()
+	return u != nil && u.Role == user.RoleAdmin
+}
+
 // IP returns the visitor IP address
 func (v *visitor) IP() netip.Addr {
 	v.mu.Lock()