tweaks to api and docs

In particular, the docs now talk about the CSRF protection. This is
increasing relevant as we start having more mutation endpoints. And
make the signals api expect a csrf for session auth to match the newer
users api.

CHANGELOG.md

@@ -6,6 +6,11 @@ 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
@@ -20,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

@@ -23,13 +23,15 @@ 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)
 
 ## Summary
 
@@ -91,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):
 
@@ -192,8 +193,6 @@ The `application/json` response will have a JSON object as follows:
     *   `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:
@@ -430,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
@@ -720,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.
 
@@ -738,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
@@ -832,73 +832,80 @@ 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 object 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`: see `Permissions` below.
+
-*   `password` (optional): a string.
+*   `csrf`: a CSRF token, required when using session authentication.
-*   `preferences` (optional): an arbitrary JSON object. Interpretation is
+*   `user`: a `UserSubset` as defined below.
-    up to clients.
 
 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 object.
+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`: see `Permissions` below.
 
 #### `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 object.
+        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`, a `Permissions` as described below, which always requires
+    values.
-  `admin_users` permission to update. Note that updating a user's permissions
-  currently neither adds nor limits permissions of existing sessions; it only
-  changes what is available to newly created sessions.
 
 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:
@@ -911,8 +918,46 @@ A JSON object of permissions to perform various actions:
 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

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;
 
@@ -123,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,
@@ -516,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)]
@@ -598,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>,

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(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,
@@ -106,10 +111,18 @@ impl Service {
         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,13 +150,7 @@ 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) {
                 bail_t!(FailedPrecondition, "username mismatch");