From be4e11c506343ef5469eaac2dff3c662cc07c898 Mon Sep 17 00:00:00 2001 From: Scott Lamb Date: Fri, 23 Dec 2022 15:43:00 -0500 Subject: [PATCH] extend POST /users/:id Now you can set a password for a user while the server is running, e.g. via the following command: ```shell curl \ -H 'Content-Type: application/json' \ -d '{"update": {"password": "asdf"}}' \ --unix-socket /var/lib/moonfire-nvr/sock \ http://nvr/api/users/1 ``` --- design/api.md | 24 ++++++++----- server/db/auth.rs | 65 ++++++++++++++++++++++++----------- server/db/db.rs | 4 +++ server/db/proto/schema.proto | 4 +++ server/src/cmds/run/config.rs | 4 +++ server/src/json.rs | 28 ++++++++++++--- server/src/web/mod.rs | 58 ++++++++++++++++++++++++------- 7 files changed, 141 insertions(+), 46 deletions(-) diff --git a/design/api.md b/design/api.md index 7b32886..179b19a 100644 --- a/design/api.md +++ b/design/api.md @@ -4,8 +4,9 @@ Status: **current**. * [Objective](#objective) * [Detailed design](#detailed-design) - * [`POST /api/login`](#post-apilogin) - * [`POST /api/logout`](#post-apilogout) + * [Authentication](#authentication) + * [`POST /api/login`](#post-apilogin) + * [`POST /api/logout`](#post-apilogout) * [`GET /api/`](#get-api) * [`GET /api/cameras//`](#get-apicamerasuuid) * [`GET /api/cameras///recordings`](#get-apicamerasuuidstreamrecordings) @@ -50,7 +51,9 @@ developed tools. All requests for JSON data should be sent with the header `Accept: application/json` (exactly). -### `POST /api/login` +### Authentication + +#### `POST /api/login` The request should have an `application/json` body containing a JSON object with `username` and `password` keys. @@ -63,7 +66,7 @@ If authentication or authorization fails, the server will return a HTTP 403 (forbidden) response. Currently the body will be a `text/plain` error message; future versions will likely be more sophisticated. -### `POST /api/logout` +#### `POST /api/logout` The request should have an `application/json` body containing a `csrf` parameter copied from the `session.csrf` of the @@ -821,17 +824,22 @@ Response: ### `POST /api/users/` -Currently this request only allows updating the preferences for the -currently-authenticated user. This is likely to change. +Allows updating the given user. Requires the `admin_users` permission if the +caller is 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 * `precondition`: forces the request to fail with HTTP status 412 (Precondition failed) if the provided fields don't have the given value. -Currently both objects support a single field, `preferences`, which should be -a JSON dictionary. +Currently the following fields are supported for `update` and `precondition`: + +* `preferences`, a JSON dictionary. +* `password`, a cleartext string. When updating the password, the previous + password must be supplied as a precondition, unless the caller has + `admin_users` permission. Returns HTTP status 204 (No Content) on success. diff --git a/server/db/auth.rs b/server/db/auth.rs index 179fff3..3b303bb 100644 --- a/server/db/auth.rs +++ b/server/db/auth.rs @@ -62,6 +62,42 @@ impl User { pub fn has_password(&self) -> bool { self.password_hash.is_some() } + + /// Checks if the user's password hash matches the supplied password. + /// + /// As a side effect, increments `password_failure_count` and sets `dirty` + /// if `password` is incorrect. + pub fn check_password(&mut self, password: Option<&str>) -> Result { + let hash = self.password_hash.as_ref(); + let (password, hash) = match (password, hash) { + (None, None) => return Ok(true), + (Some(p), Some(h)) => (p, h), + _ => return Ok(false), + }; + let hash = PasswordHash::new(hash) + .with_context(|_| { + format!( + "bad stored password hash for user {:?}: {:?}", + self.username, hash + ) + }) + .context(ErrorKind::DataLoss)?; + match scrypt::Scrypt.verify_password(password.as_bytes(), &hash) { + Ok(()) => Ok(true), + Err(scrypt::password_hash::errors::Error::Password) => { + self.dirty = true; + self.password_failure_count += 1; + Ok(false) + } + Err(e) => Err(e + .context(format!( + "unable to verify password for user {:?}", + self.username + )) + .context(ErrorKind::Internal) + .into()), + } + } } /// A change to a user. @@ -387,6 +423,10 @@ impl State { &self.users_by_id } + pub fn get_user_by_id_mut(&mut self, id: i32) -> Option<&mut User> { + self.users_by_id.get_mut(&id) + } + fn update_user( &mut self, conn: &Connection, @@ -527,26 +567,9 @@ impl State { if u.config.disabled { bail!("user {:?} is disabled", username); } - let hash = u - .password_hash - .as_ref() - .ok_or_else(|| format_err!("no password set for user {:?}", username))?; - let hash = PasswordHash::new(hash) - .with_context(|_| format!("bad stored password hash for user {:?}", username))?; - match scrypt::Scrypt.verify_password(password.as_bytes(), &hash) { - Ok(()) => {} - Err(scrypt::password_hash::errors::Error::Password) => { - u.dirty = true; - u.password_failure_count += 1; - bail!("incorrect password for user {:?}", username); - } - Err(e) => { - return Err(e - .context(format!("unable to verify password for user {:?}", username)) - .into()); - } + if !u.check_password(Some(&password))? { + bail_t!(Unauthenticated, "incorrect password"); } - let password_id = u.password_id; State::make_session_int( &self.rand, @@ -924,7 +947,7 @@ mod tests { 0, ) .unwrap_err(); - assert_eq!(format!("{}", e), "no password set for user \"slamb\""); + assert_eq!(format!("{}", e), "Unauthenticated: incorrect password"); c.set_password("hunter2".to_owned()); state.apply(&conn, c).unwrap(); let e = state @@ -937,7 +960,7 @@ mod tests { 0, ) .unwrap_err(); - assert_eq!(format!("{}", e), "incorrect password for user \"slamb\""); + assert_eq!(format!("{}", e), "Unauthenticated: incorrect password"); let sid = { let (sid, s) = state .login_by_password( diff --git a/server/db/db.rs b/server/db/db.rs index 4f8d9b1..24b7738 100644 --- a/server/db/db.rs +++ b/server/db/db.rs @@ -2034,6 +2034,10 @@ impl LockedDatabase { self.auth.users_by_id() } + pub fn get_user_by_id_mut(&mut self, id: i32) -> Option<&mut User> { + self.auth.get_user_by_id_mut(id) + } + pub fn apply_user_change(&mut self, change: UserChange) -> Result<&User, Error> { self.auth.apply(&self.conn, change) } diff --git a/server/db/proto/schema.proto b/server/db/proto/schema.proto index 681b0c5..8b14a4e 100644 --- a/server/db/proto/schema.proto +++ b/server/db/proto/schema.proto @@ -72,4 +72,8 @@ message Permissions { 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; } diff --git a/server/src/cmds/run/config.rs b/server/src/cmds/run/config.rs index 2aa9d21..2c8bdf1 100644 --- a/server/src/cmds/run/config.rs +++ b/server/src/cmds/run/config.rs @@ -95,6 +95,9 @@ pub struct Permissions { #[serde(default)] update_signals: bool, + + #[serde(default)] + admin_users: bool, } impl Permissions { @@ -103,6 +106,7 @@ impl Permissions { view_video: self.view_video, read_camera_configs: self.read_camera_configs, update_signals: self.update_signals, + admin_users: self.admin_users, ..Default::default() } } diff --git a/server/src/json.rs b/server/src/json.rs index c00adf0..52cb35d 100644 --- a/server/src/json.rs +++ b/server/src/json.rs @@ -6,7 +6,7 @@ use base::time::{Duration, Time}; use db::auth::SessionHash; use failure::{format_err, Error}; use serde::ser::{Error as _, SerializeMap, SerializeSeq, Serializer}; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize}; use std::ops::Not; use uuid::Uuid; @@ -513,13 +513,31 @@ pub struct ToplevelUser { #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct PostUser { - pub update: Option, - pub precondition: Option, +pub struct PostUser<'a> { + pub csrf: Option<&'a str>, + pub update: Option>, + pub precondition: Option>, } #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct UserSubset { +pub struct UserSubset<'a> { pub preferences: Option, + + /// An optional password value. + /// + /// `None` indicates the password does not wish to check/update the password. + /// `Some(None)` indicates the password should be absent. + #[serde(borrow, default, deserialize_with = "deserialize_some")] + pub password: Option>, +} + +// Any value that is present is considered Some value, including null. +// https://github.com/serde-rs/serde/issues/984#issuecomment-314143738 +fn deserialize_some<'de, T, D>(deserializer: D) -> Result, D::Error> +where + T: Deserialize<'de>, + D: Deserializer<'de>, +{ + Deserialize::deserialize(deserializer).map(Some) } diff --git a/server/src/web/mod.rs b/server/src/web/mod.rs index 6ef6ce8..a69eb8d 100644 --- a/server/src/web/mod.rs +++ b/server/src/web/mod.rs @@ -15,8 +15,7 @@ use self::path::Path; use crate::body::Body; use crate::json; use crate::mp4; -use base::{bail_t, ErrorKind}; -use base::{clock::Clocks, format_err_t}; +use base::{bail_t, clock::Clocks, format_err_t, ErrorKind}; use core::borrow::Borrow; use core::str::FromStr; use db::dir::SampleFileDir; @@ -82,7 +81,8 @@ fn from_base_error(err: base::Error) -> Response { let status_code = match err.kind() { Unauthenticated => StatusCode::UNAUTHORIZED, PermissionDenied => StatusCode::FORBIDDEN, - InvalidArgument | FailedPrecondition => StatusCode::BAD_REQUEST, + InvalidArgument => StatusCode::BAD_REQUEST, + FailedPrecondition => StatusCode::PRECONDITION_FAILED, NotFound => StatusCode::NOT_FOUND, _ => StatusCode::INTERNAL_SERVER_ERROR, }; @@ -462,33 +462,66 @@ impl Service { } async fn user(&self, req: Request, caller: Caller, id: i32) -> ResponseResult { - if caller.user.map(|u| u.id) != Some(id) { - bail_t!(Unauthenticated, "must be authenticated as supplied user"); + if caller.user.as_ref().map(|u| u.id) != Some(id) && !caller.permissions.admin_users { + bail_t!( + Unauthenticated, + "must be authenticated as supplied user or have admin_users permission" + ); } match *req.method() { - Method::POST => self.post_user(req, id).await, + Method::POST => self.post_user(req, caller, id).await, _ => Err(plain_response(StatusCode::METHOD_NOT_ALLOWED, "POST expected").into()), } } - async fn post_user(&self, mut req: Request, id: i32) -> ResponseResult { + async fn post_user( + &self, + mut req: Request, + caller: Caller, + id: i32, + ) -> ResponseResult { let r = extract_json_body(&mut req).await?; let r: json::PostUser = serde_json::from_slice(&r).map_err(|e| bad_req(e.to_string()))?; let mut db = self.db.lock(); let user = db - .users_by_id() - .get(&id) - .ok_or_else(|| format_err_t!(Internal, "can't find currently authenticated user"))?; - if let Some(precondition) = r.precondition { - if matches!(precondition.preferences, Some(p) if p != user.config.preferences) { + .get_user_by_id_mut(id) + .ok_or_else(|| format_err_t!(Internal, "can't find requested user"))?; + if r.update.as_ref().map(|u| u.password).is_some() + && r.precondition.as_ref().map(|p| p.password).is_none() + && !caller.permissions.admin_users + { + bail_t!( + Unauthenticated, + "to change password, must supply previous password or have admin_users permission" + ); + } + match (r.csrf, caller.user.and_then(|u| u.session)) { + (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(ref precondition) = r.precondition { + if matches!(precondition.preferences, Some(ref p) if p != &user.config.preferences) { bail_t!(FailedPrecondition, "preferences mismatch"); } + if let Some(p) = precondition.password { + if !user.check_password(p)? { + bail_t!(FailedPrecondition, "password mismatch"); // or Unauthenticated? + } + } } if let Some(update) = r.update { let mut change = user.change(); if let Some(preferences) = update.preferences { change.config.preferences = preferences; } + match update.password { + None => {} + Some(None) => change.clear_password(), + Some(Some(p)) => change.set_password(p.to_owned()), + } db.apply_user_change(change).map_err(internal_server_err)?; } Ok(plain_response(StatusCode::NO_CONTENT, &b""[..])) @@ -611,6 +644,7 @@ impl Service { view_video: true, read_camera_configs: true, update_signals: true, + admin_users: true, ..Default::default() }, user: None,