Merge branch 'master' into increment-seq

CHANGELOG.md

@@ -6,8 +6,15 @@ changes, see Git history.
 Each release is tagged in Git and on the Docker repository
 [`scottlamb/moonfire-nvr`](https://hub.docker.com/r/scottlamb/moonfire-nvr).
 
+Backwards-incompatible database schema changes happen on on major version
+upgrades, e.g. `0.6.x` -> `0.7.x`. The config file format and
+[API](design/api.md) currently have no stability guarantees, so they may change
+even on minor releases, e.g. `0.7.5` -> `0.7.6`.
+
 ## unreleased
 
+*   expect camelCase in `moonfire-nvr.toml` file, for consistency with the JSON
+    API. You'll need to adjust your config file when upgrading.
 *   use Retina 0.4.3, which is newly compatible with rtsp-simple-server v0.19.3
     and some TP-Link cameras. Fixes [#238](https://github.com/scottlamb/moonfire-nvr/issues/238).
 *   expanded API interface for examining and updating users:
@@ -18,6 +25,7 @@ Each release is tagged in Git and on the Docker repository
     *   expanded `POST /users/<id>` endpoint, including password and
         permissions.
     *   `DELETE /users/<id>` endpoint to delete a user
+*   improved API documentation in `design/api.md`.
 
 ## 0.7.5 (2022-05-09)
 

design/api.md

@@ -2,8 +2,8 @@
 
 Status: **current**.
 
-* [Objective](#objective)
+* [Summary](#summary)
-* [Detailed design](#detailed-design)
+* [Endpoints](#endpoints)
     * [Authentication](#authentication)
         * [`POST /api/login`](#post-apilogin)
         * [`POST /api/logout`](#post-apilogout)
@@ -23,16 +23,20 @@ Status: **current**.
         * [Request 2](#request-2)
         * [Request 3](#request-3)
     * [User management](#user-management)
-        * [`GET /api/users`](#get-apiusers)
+        * [`GET /api/users/`](#get-apiusers)
-        * [`PUT /api/users`](#put-apiusers)
+        * [`PUT /api/users/`](#put-apiusers)
         * [`GET /api/users/<id>`](#get-apiusersid)
         * [`POST /api/users/<id>`](#post-apiusersid)
         * [`DELETE /api/users/<id>`](#delete-apiusersid)
+* [Types](#types)
+    * [UserSubset](#usersubset)
+    * [Permissions](#permissions)
+* [Cross-site request forgery (CSRF) protection](#cross-site-request-forgery-csrf-protection)
 
-## Objective
+## Summary
 
-Allow a JavaScript-based web interface to list cameras and view recordings.
+A JavaScript-based web interface to list cameras and view recordings.
-Support external analytics.
+Supports external analytics.
 
 In the future, this is likely to be expanded:
 
@@ -40,8 +44,6 @@ In the future, this is likely to be expanded:
 *   commandline tool over a UNIX-domain socket
     (at least for bootstrapping web authentication)
 
-## Detailed design
-
 *Note:* italicized terms in this document are defined in the [glossary](glossary.md).
 
 Currently the API is considered an internal contract between the server and the
@@ -56,6 +58,8 @@ developed tools.
 All requests for JSON data should be sent with the header
 `Accept: application/json` (exactly).
 
+## Endpoints
+
 ### Authentication
 
 #### `POST /api/login`
@@ -89,8 +93,7 @@ request parameters:
     should be included.
 *   `cameraConfigs`: a boolean indicating if the `camera.config` and
     `camera.stream[].config` parameters described below should be included.
-    This requires the `read_camera_configs` permission as described in
+    This requires the `readCameraConfigs` permission.
-    `schema.proto`.
 
 Example request URI (with added whitespace between parameters):
 
@@ -184,13 +187,12 @@ The `application/json` response will have a JSON object as follows:
             considered to have motion when this signal is in this state.
         *   `color` (optional): a recommended color to use in UIs to represent
             this state, as in the [HTML specification](https://html.spec.whatwg.org/#colours).
+*   `permissions`: the caller's current `Permissions` object (defined below).
 *   `user`: an object, present only when authenticated:
     *   `name`: a human-readable name
     *   `id`: an integer
     *   `preferences`: a JSON object
     *   `session`: an object, present only if authenticated via session cookie.
-        (In the future, it will be possible to instead authenticate via uid over
-        a Unix domain socket.)
         *   `csrf`: a cross-site request forgery token for use in `POST` requests.
 
 Example response:
@@ -427,7 +429,7 @@ Example response:
 
 ### `GET /api/cameras/<uuid>/<stream>/view.mp4`
 
-Requires the `view_video` permission.
+Requires the `viewVideo` permission.
 
 Returns a `.mp4` file, with an etag and support for range requests. The MIME
 type will be `video/mp4`, with a `codecs` parameter as specified in
@@ -717,7 +719,7 @@ This represents the following observations:
 
 ### `POST /api/signals`
 
-Requires the `update_signals` permission.
+Requires the `updateSignals` permission.
 
 Alters the state of a signal.
 
@@ -735,6 +737,7 @@ last ran. These will specify beginning and end times.
 The request should have an `application/json` body describing the change to
 make. It should be a JSON object with these attributes:
 
+*   `csrf`: a CSRF token, required when using session authentication.
 *   `signalIds`: a list of signal ids to change. Must be sorted.
 *   `states`: a list (one per `signalIds` entry) of states to set.
 *   `start`: the starting time of the change, as a JSON object of the form
@@ -829,69 +832,132 @@ Response:
 
 ### User management
 
-#### `GET /api/users`
+#### `GET /api/users/`
 
-Requires the `admin_users` permission.
+Requires the `adminUsers` permission.
 
 Lists all users. Currently there's no paging. Returns a JSON object with
-a `users` key with a map of id to username.
+a `users` key with an array of objects, each with the following keys:
 
-#### `PUT /api/users`
+*   `id`: a number.
+*   `username`: a string.
 
-Requires the `admin_users` permission.
+#### `PUT /api/users/`
 
-Adds a user. Expects a JSON dictionary with the parameters for the user:
+Requires the `adminUsers` permission.
 
-*   `username`: a string, which must be unique.
+Adds a user. Expects a JSON object as follows:
-*   `permissions`: a JSON dictionary of permissions.
+
-*   `password` (optional): a string.
+*   `csrf`: a CSRF token, required when using session authentication.
-*   `preferences` (optional): a JSON dictionary.
+*   `user`: a `UserSubset` as defined below.
 
 Returns status 204 (No Content) on success.
 
 #### `GET /api/users/<id>`
 
-Retrieves the user. Requires the `admin_users` permission if the caller is
+Retrieves the user. Requires the `adminUsers` permission if the caller is
 not authenticated as the user in question.
 
-Returns a HTTP status 200 on success with a JSON dict:
+Returns a HTTP status 200 on success with a JSON `UserSubset`. The `password`
-
+will be absent (for no password) or a placeholder string to indicate the
-*   `preferences`: a JSON dictionary.
+password is set. Passwords are stored hashed, so the cleartext can not be
-*   `password`: absent (no password set) or a placeholder string to indicate
+retrieved.
-    the password is set. Passwords are stored hashed, so the cleartext can not
-    be retrieved.
-*   `permissions`.
 
 #### `POST /api/users/<id>`
 
-Allows updating the given user. Requires the `admin_users` permission if the
+Updates the given user. Requires the `adminUsers` permission if the caller is
-caller is not authenticated as the user in question.
+not authenticated as the user in question.
 
 Expects a JSON object:
 
 *   `csrf`: a CSRF token, required when using session authentication.
-*   `update`: sets the provided fields
+*   `update`: `UserSubset`, sets the provided fields. Field-specific notes:
-*   `precondition`: forces the request to fail with HTTP status 412
+    *   `password`: when updating the password, the previous password must
-    (Precondition failed) if the provided fields don't have the given value.
+        be supplied as a precondition, unless the caller has `admin_users`
-
+        permission.
-Currently the following fields are supported for `update` and `precondition`:
+    *   `permissions`: requires `adminUsers` permission. Note that updating a
-
+        user's permissions currently neither adds nor limits permissions of
-* `preferences`, a JSON dictionary.
+        existing sessions; it only changes what is available to newly created
-* `password`, a cleartext string. When updating the password, the previous
+        sessions.
-  password must be supplied as a precondition, unless the caller has
+*   `precondition`: `UserSubset`, forces the request to fail with HTTP status
-  `admin_users` permission.
+    412 (Precondition failed) if the provided fields don't have the given
-* `permissions`, which always requires `admin_users` permission to update.
+    values.
 
 Returns HTTP status 204 (No Content) on success.
 
 #### `DELETE /api/users/<id>`
 
-Deletes the given user. Requires the `admin_users` permission.
+Deletes the given user. Requires the `adminUsers` permission.
+
+Expects a JSON object body with the following parameters:
+
+*   `csrf`: a CSRF token, required when using session authentication.
 
 Returns HTTP status 204 (No Content) on success.
 
+## Types
+
+### UserSubset
+
+A JSON object with any of the following parameters:
+
+*   `preferences`, a JSON object which the server stores without interpreting.
+    This field is meant for user-level preferences meaningful to the UI.
+*   `password`, a cleartext string.
+*   `permissions`, a `Permissions` as described below.
+
+### Permissions
+
+A JSON object of permissions to perform various actions:
+
+*   `adminUsers`: bool
+*   `readCameraConfigs`: bool, read camera configs including credentials
+*   `updateSignals`: bool
+*   `viewVideo`: bool
+
+See endpoints above for more details on the contexts in which these are
+required.
+
+## Cross-site request forgery (CSRF) protection
+
+The API includes several standard protections against [cross-site request
+forgery](https://en.wikipedia.org/wiki/Cross-site_request_forgery) attacks, in
+which a malicious third-party website convinces the user's browser to send
+requests on its behalf.
+
+The following protections apply regardless of method of authentication (session,
+no authentication + `allowUnauthenticatedPermissions`, or the browser proxying
+to a Unix socket with `ownUidIsPrivileged`):
+
+*   The `GET` method is always "safe". Actions that have significant side
+    effects require another method such as `DELETE`, `POST`, or `PUT`. This
+    prevents simple hyperlinks from causing damage.
+*   Mutations always require some non-default request header (e.g.
+    `Content-Type: application/json`) so that a `<form method="POST">` will be
+    rejected.
+*   The server does *not* override the default
+    [CORS](https://en.wikipedia.org/wiki/Cross-origin_resource_sharing) policy.
+    Thus, cross-domain Ajax requests (via `XMLHTTPRequest` or `fetch`) should
+    fail.
+*   WebSocket upgrade requests are rejected unless the `Origin` and `Host`
+    headers match.
+
+The following additional protections apply only when using session
+authentication:
+
+*   Session cookies are set with the [`SameSite=Lax` attribute](samesite-lax),
+    so that sufficiently modern web browsers will never send the session cookie
+    on subrequests. (Note they still send session cookies when following links
+    and on WebSocket upgrade. In these cases, we rely on the protections
+    described above.)
+*   Mutations use a `csrf` token, requiring the caller to prove it is able
+    to read the `GET /api/` response. (This is subect to change. We may decide
+    to implement these tokens in a way that doesn't require session
+    authentication or decide they're entirely unnecessary.)
+
 [media-segment]: https://w3c.github.io/media-source/isobmff-byte-stream-format.html#iso-media-segments
 [init-segment]: https://w3c.github.io/media-source/isobmff-byte-stream-format.html#iso-init-segments
 [rfc-6381]: https://tools.ietf.org/html/rfc6381
 [rfc-6455]: https://tools.ietf.org/html/rfc6455
 [multipart-mixed-js]: https://github.com/scottlamb/multipart-mixed-js
+[samesite-lax]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#lax

design/signal.md

@@ -0,0 +1,35 @@
+# Moonfire NVR Signals
+
+Status: **draft**.
+
+"Signals" are what Moonfire NVR uses to describe non-video timeseries data
+such as "was motion detected?" or "what mode was my burglar alarm in?" They are
+intended to be displayed in the UI with the video scrub bar to aid in finding
+a relevant portion of video.
+
+## Objective
+
+Goals:
+
+*   represent simple results of on-camera and on-NVR motion detection, e.g.:
+    `true`, `false`, or `unknown`.
+*   represent external signals such as burglar alarm state, e.g.:
+    `off`, `stay`, `away`, `alarm`, or `unknown`.
+
+Non-goals:
+
+*   provide meaningful data when the NVR has inaccurate system time.
+*   support internal state necessary for on-NVR motion detection. (This will
+    be considered separately.)
+*   support fine-grained outputs such as "what are the bounding boxes of all
+    detected faces?", "what cells have motion?", audio volume, or audio
+    spectograms.
+
+## Overview
+
+hmm, two ideas:
+
+*   just use timestamps everywhere. allow adding/updating historical data.
+*   only allow updating the current open. initially, just support setting
+    current time. then support extending from a previous request. no ability
+    to fill in while NVR is down.

docker/dev.bash

@@ -90,7 +90,7 @@ time apt-get install --assume-yes --no-install-recommends "${packages[@]}"
 # https://doc.rust-lang.org/cargo/guide/build-cache.html
 if [[ -n "${rust_target}" ]]; then
     su moonfire-nvr -lc "rustup target install ${rust_target} &&
-                         ln -s target/${rust_target}/release/moonfire-nvr ."
+                         ln -s target/${rust_target}/release-lto/moonfire-nvr ."
     underscore_rust_target="${rust_target//-/_}"
     uppercase_underscore_rust_target="${underscore_rust_target^^}"
     cat >> /var/lib/moonfire-nvr/.buildrc <<EOF
@@ -112,7 +112,7 @@ export CC_${underscore_rust_target}=${gcc_target}-gcc
 export CXX_${underscore_rust_target}=${gcc_target}-g++
 EOF
 else
-    su moonfire-nvr -lc "ln -s target/release/moonfire-nvr ."
+    su moonfire-nvr -lc "ln -s target/release-lto/moonfire-nvr ."
 fi
 
 ls -laFR /var/cache/apt > /docker-build-debug/dev/var-cache-apt-after

guide/build.md

@@ -295,11 +295,11 @@ You'll also need a `/etc/moonfire-nvr.toml`:
 ```toml
 [[binds]]
 ipv4 = "0.0.0.0:8080"
-allow_unauthenticated_permissions = { view_video = true }
+allowUnauthenticatedPermissions = { viewVideo = true }
 
 [[binds]]
 unix = "/var/lib/moonfire-nvr/sock"
-own_uid_is_privileged = true
+ownUidIsPrivileged = true
 ```
 
 Note this configuration is insecure. You can change that via replacing the

guide/install.md

@@ -71,11 +71,11 @@ $ sudo chmod a+rx /usr/local/bin/nvr
 ```toml
 [[binds]]
 ipv4 = "0.0.0.0:8080"
-allow_unauthenticated_permissions = { view_video = true }
+allowUnauthenticatedPermissions = { viewVideo = true }
 
 [[binds]]
 unix = "/var/lib/moonfire-nvr/sock"
-own_uid_is_privileged = true
+ownUidIsPrivileged = true
 ```
 
 `/usr/local/bin/nvr`:

guide/secure.md

@@ -165,13 +165,13 @@ If you follow the recommended Docker setup, your `/etc/moonfire-nvr.json`
 will contain this line:
 
 ```toml
-allow_unauthenticated_permissions = { view_video = true }
+allowUnauthenticatedPermissions = { viewVideo = true }
 ```
 
 Replace it with the following:
 
 ```toml
-trust_forward_headers = true
+trustForwardHeaders = true
 ```
 
 This change has two effects:

server/db/proto/schema.proto

@@ -2,6 +2,11 @@
 // Copyright (C) 2018 The Moonfire NVR Authors; see AUTHORS and LICENSE.txt.
 // SPDX-License-Identifier: GPL-v3.0-or-later WITH GPL-3.0-linking-exception.';
 
+// Protobuf portion of the Moonfire NVR schema. In general Moonfire's schema
+// uses a SQLite3 database with some fields in JSON representation. The protobuf
+// stuff is just high-cardinality things that must be compact, e.g. permissions
+// that can be stuffed into every user session.
+
 syntax = "proto3";
 
 // Metadata stored in sample file dirs as `<dir>/meta`. This is checked
@@ -55,25 +60,12 @@ message DirMeta {
   Open in_progress_open = 4;
 }
 
-// Permissions to perform actions, currently all simple bools.
+// Permissions to perform actions. See description in design/api.md.
 //
-// These indicate actions which may be unnecessary in some contexts. Some
+// This protobuf form is stored in user and session rows.
-// basic access - like listing the cameras - is currently always allowed.
-// See design/api.md for a description of what requires these permissions.
-//
-// These are used in a few contexts:
-// * a session - affects what can be done when using that session to
-//   authenticate.
-// * a user - when a new session is created, it inherits these permissions.
-// * on the commandline - to specify what permissions are available for
-//   unauthenticated access.
 message Permissions {
   bool view_video = 1;
   bool read_camera_configs = 2;
-
   bool update_signals = 3;
-
-  // Administrate user accounts: create, delete accounts; modify passwords of
-  // accounts other than the caller's own.
   bool admin_users = 4;
 }

server/src/cmds/run/config.rs

@@ -21,6 +21,7 @@ fn default_ui_dir() -> PathBuf {
 /// Top-level configuration file object.
 #[derive(Debug, Deserialize)]
 #[serde(deny_unknown_fields)]
+#[serde(rename_all = "camelCase")]
 pub struct ConfigFile {
     pub binds: Vec<BindConfig>,
 
@@ -42,6 +43,7 @@ pub struct ConfigFile {
 /// Per-bind configuration.
 #[derive(Debug, Deserialize)]
 #[serde(deny_unknown_fields)]
+#[serde(rename_all = "camelCase")]
 pub struct BindConfig {
     /// The address to bind to.
     #[serde(flatten)]
@@ -70,8 +72,8 @@ pub struct BindConfig {
 }
 
 #[derive(Debug, Deserialize)]
-#[serde(rename_all = "lowercase")]
 #[serde(deny_unknown_fields)]
+#[serde(rename_all = "camelCase")]
 pub enum AddressConfig {
     /// IPv4 address such as `0.0.0.0:8080` or `127.0.0.1:8080`.
     Ipv4(std::net::SocketAddrV4),

server/src/cmds/run/mod.rs

@@ -367,7 +367,7 @@ async fn inner(
                 ui_dir: Some(&config.ui_dir),
                 allow_unauthenticated_permissions: b
                     .allow_unauthenticated_permissions
-                    .as_ref()
+                    .clone()
                     .map(db::Permissions::from),
                 trust_forward_hdrs: b.trust_forward_headers,
                 time_zone_name: time_zone_name.clone(),

server/src/json.rs

@@ -9,7 +9,6 @@ use db::auth::SessionHash;
 use failure::{format_err, Error};
 use serde::ser::{Error as _, SerializeMap, SerializeSeq, Serializer};
 use serde::{Deserialize, Deserializer, Serialize};
-use std::collections::BTreeMap;
 use std::ops::Not;
 use uuid::Uuid;
 
@@ -25,6 +24,8 @@ pub struct TopLevel<'a> {
     #[serde(serialize_with = "TopLevel::serialize_cameras")]
     pub cameras: (&'a db::LockedDatabase, bool, bool),
 
+    pub permissions: Permissions,
+
     #[serde(skip_serializing_if = "Option::is_none")]
     pub user: Option<ToplevelUser>,
 
@@ -121,12 +122,15 @@ pub struct LoginRequest<'a> {
 #[derive(Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub struct LogoutRequest<'a> {
+    #[serde(borrow)]
     pub csrf: &'a str,
 }
 
 #[derive(Deserialize)]
 #[serde(rename_all = "camelCase")]
-pub struct PostSignalsRequest {
+pub struct PostSignalsRequest<'a> {
+    #[serde(borrow)]
+    pub csrf: Option<&'a str>,
     pub signal_ids: Vec<u32>,
     pub states: Vec<u16>,
     pub start: PostSignalsTimeBase,
@@ -514,15 +518,33 @@ pub struct ToplevelUser {
     pub session: Option<Session>,
 }
 
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "camelCase")]
+#[serde(deny_unknown_fields)]
+pub struct PutUsers<'a> {
+    #[serde(borrow)]
+    pub csrf: Option<&'a str>,
+    pub user: UserSubset<'a>,
+}
+
 #[derive(Debug, Deserialize)]
 #[serde(rename_all = "camelCase")]
 #[serde(deny_unknown_fields)]
 pub struct PostUser<'a> {
+    #[serde(borrow)]
     pub csrf: Option<&'a str>,
     pub update: Option<UserSubset<'a>>,
     pub precondition: Option<UserSubset<'a>>,
 }
 
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "camelCase")]
+#[serde(deny_unknown_fields)]
+pub struct DeleteUser<'a> {
+    #[serde(borrow)]
+    pub csrf: Option<&'a str>,
+}
+
 #[derive(Debug, Default, Deserialize, Serialize, PartialEq, Eq)]
 #[serde(rename_all = "camelCase")]
 #[serde(deny_unknown_fields)]
@@ -553,8 +575,9 @@ where
 }
 
 /// API/config analog of `Permissions` defined in `db/proto/schema.proto`.
-#[derive(Debug, Default, Deserialize, Serialize, PartialEq, Eq)]
+#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq, Eq)]
 #[serde(deny_unknown_fields)]
+#[serde(rename_all = "camelCase")]
 pub struct Permissions {
     #[serde(default)]
     view_video: bool,
@@ -569,8 +592,8 @@ pub struct Permissions {
     admin_users: bool,
 }
 
-impl From<&Permissions> for db::schema::Permissions {
+impl From<Permissions> for db::schema::Permissions {
-    fn from(p: &Permissions) -> Self {
+    fn from(p: Permissions) -> Self {
         Self {
             view_video: p.view_video,
             read_camera_configs: p.read_camera_configs,
@@ -581,8 +604,8 @@ impl From<&Permissions> for db::schema::Permissions {
     }
 }
 
-impl From<&db::schema::Permissions> for Permissions {
+impl From<db::schema::Permissions> for Permissions {
-    fn from(p: &db::schema::Permissions) -> Self {
+    fn from(p: db::schema::Permissions) -> Self {
         Self {
             view_video: p.view_video,
             read_camera_configs: p.read_camera_configs,
@@ -595,7 +618,13 @@ impl From<&db::schema::Permissions> for Permissions {
 /// Response to `GET /api/users/`.
 #[derive(Serialize)]
 pub struct GetUsersResponse {
-    pub users: BTreeMap<i32, String>,
+    pub users: Vec<UserSummary>,
+}
+
+#[derive(Serialize)]
+pub struct UserSummary {
+    pub id: i32,
+    pub username: String,
 }
 
 /// Response to `PUT /api/users/`.

server/src/web/mod.rs

@@ -153,6 +153,16 @@ async fn extract_json_body(req: &mut Request<hyper::Body>) -> Result<Bytes, Http
         .map_err(|e| internal_server_err(format_err!("unable to read request body: {}", e)))
 }
 
+fn require_csrf_if_session(caller: &Caller, csrf: Option<&str>) -> Result<(), base::Error> {
+    match (csrf, caller.user.as_ref().and_then(|u| u.session.as_ref())) {
+        (None, Some(_)) => bail_t!(Unauthenticated, "csrf must be supplied"),
+        (Some(csrf), Some(session)) if !csrf_matches(csrf, session.csrf) => {
+            bail_t!(Unauthenticated, "incorrect csrf");
+        }
+        (_, _) => Ok(()),
+    }
+}
+
 pub struct Config<'a> {
     pub db: Arc<db::Database>,
     pub ui_dir: Option<&'a std::path::Path>,
@@ -354,6 +364,7 @@ impl Service {
                 user: caller.user,
                 signals: (&db, days),
                 signal_types: &db,
+                permissions: caller.permissions.into(),
             },
         )
     }

server/src/web/signals.rs

@@ -12,8 +12,8 @@ use url::form_urlencoded;
 use crate::json;
 
 use super::{
-    bad_req, extract_json_body, from_base_error, plain_response, serve_json, Caller,
+    bad_req, extract_json_body, from_base_error, plain_response, require_csrf_if_session,
-    ResponseResult, Service,
+    serve_json, Caller, ResponseResult, Service,
 };
 
 use std::borrow::Borrow;
@@ -42,6 +42,7 @@ impl Service {
         let r = extract_json_body(&mut req).await?;
         let r: json::PostSignalsRequest =
             serde_json::from_slice(&r).map_err(|e| bad_req(e.to_string()))?;
+        require_csrf_if_session(&caller, r.csrf)?;
         let now = recording::Time::new(self.db.clocks().realtime());
         let mut l = self.db.lock();
         let start = match r.start {

server/src/web/users.rs

@@ -7,11 +7,11 @@
 use base::{bail_t, format_err_t};
 use http::{Method, Request, StatusCode};
 
-use crate::json::{self, PutUsersResponse, UserSubset};
+use crate::json::{self, PutUsersResponse, UserSubset, UserSummary};
 
 use super::{
-    bad_req, csrf_matches, extract_json_body, plain_response, serve_json, Caller, ResponseResult,
+    bad_req, extract_json_body, plain_response, require_csrf_if_session, serve_json, Caller,
-    Service,
+    ResponseResult, Service,
 };
 
 impl Service {
@@ -34,7 +34,10 @@ impl Service {
             .lock()
             .users_by_id()
             .iter()
-            .map(|(&id, user)| (id, user.username.clone()))
+            .map(|(&id, user)| UserSummary {
+                id,
+                username: user.username.clone(),
+            })
             .collect();
         serve_json(&req, &json::GetUsersResponse { users })
     }
@@ -44,23 +47,25 @@ impl Service {
             bail_t!(Unauthenticated, "must have admin_users permission");
         }
         let r = extract_json_body(&mut req).await?;
-        let mut r: json::UserSubset =
+        let mut r: json::PutUsers =
             serde_json::from_slice(&r).map_err(|e| bad_req(e.to_string()))?;
+        require_csrf_if_session(&caller, r.csrf)?;
         let username = r
+            .user
             .username
             .take()
             .ok_or_else(|| format_err_t!(InvalidArgument, "username must be specified"))?;
         let mut change = db::UserChange::add_user(username.to_owned());
-        if let Some(Some(pwd)) = r.password.take() {
+        if let Some(Some(pwd)) = r.user.password.take() {
             change.set_password(pwd.to_owned());
         }
-        if let Some(preferences) = r.preferences.take() {
+        if let Some(preferences) = r.user.preferences.take() {
             change.config.preferences = preferences;
         }
-        if let Some(ref permissions) = r.permissions.take() {
+        if let Some(permissions) = r.user.permissions.take() {
             change.permissions = permissions.into();
         }
-        if r != Default::default() {
+        if r.user != Default::default() {
             bail_t!(Unimplemented, "unsupported user fields: {:#?}", r);
         }
         let mut l = self.db.lock();
@@ -76,7 +81,7 @@ impl Service {
     ) -> ResponseResult {
         match *req.method() {
             Method::GET | Method::HEAD => self.get_user(req, caller, id).await,
-            Method::DELETE => self.delete_user(caller, id).await,
+            Method::DELETE => self.delete_user(req, caller, id).await,
             Method::POST => self.post_user(req, caller, id).await,
             _ => Err(plain_response(
                 StatusCode::METHOD_NOT_ALLOWED,
@@ -101,15 +106,23 @@ impl Service {
             } else {
                 None
             }),
-            permissions: Some((&user.permissions).into()),
+            permissions: Some(user.permissions.clone().into()),
         };
         serve_json(&req, &out)
     }
 
-    async fn delete_user(&self, caller: Caller, id: i32) -> ResponseResult {
+    async fn delete_user(
+        &self,
+        mut req: Request<hyper::Body>,
+        caller: Caller,
+        id: i32,
+    ) -> ResponseResult {
         if !caller.permissions.admin_users {
             bail_t!(Unauthenticated, "must have admin_users permission");
         }
+        let r = extract_json_body(&mut req).await?;
+        let r: json::DeleteUser = serde_json::from_slice(&r).map_err(|e| bad_req(e.to_string()))?;
+        require_csrf_if_session(&caller, r.csrf)?;
         let mut l = self.db.lock();
         l.delete_user(id)?;
         Ok(plain_response(StatusCode::NO_CONTENT, &b""[..]))
@@ -137,15 +150,9 @@ impl Service {
                 "to change password, must supply previous password or have admin_users permission"
             );
         }
-        match (r.csrf, caller.user.and_then(|u| u.session)) {
+        require_csrf_if_session(&caller, r.csrf)?;
-            (None, Some(_)) => bail_t!(Unauthenticated, "csrf must be supplied"),
-            (Some(csrf), Some(session)) if !csrf_matches(csrf, session.csrf) => {
-                bail_t!(Unauthenticated, "incorrect csrf");
-            }
-            (_, _) => {}
-        }
         if let Some(mut precondition) = r.precondition {
-            if matches!(precondition.username.take(), Some(n) if n != &user.username) {
+            if matches!(precondition.username.take(), Some(n) if n != user.username) {
                 bail_t!(FailedPrecondition, "username mismatch");
             }
             if matches!(precondition.preferences.take(), Some(ref p) if p != &user.config.preferences)
@@ -158,7 +165,7 @@ impl Service {
                 }
             }
             if let Some(p) = precondition.permissions.take() {
-                if user.permissions != db::Permissions::from(&p) {
+                if user.permissions != db::Permissions::from(p) {
                     bail_t!(FailedPrecondition, "permissions mismatch");
                 }
             }
@@ -193,7 +200,7 @@ impl Service {
                 change.username = n.to_string();
             }
             if let Some(permissions) = update.permissions.take() {
-                change.permissions = (&permissions).into();
+                change.permissions = permissions.into();
             }
 
             // Safety valve in case something is added to UserSubset and forgotten here.