user admin api improvements

CHANGELOG.md

@@ -23,9 +23,9 @@ even on minor releases, e.g. `0.7.5` -> `0.7.6`.
 *   expanded API interface for examining and updating users:
     *   `admin_users` permission for operating on arbitrary users.
     *   `GET /users/` endpoint to list users
-    *   `PUT /users/` endpoint to add a user
+    *   `POST /users/` endpoint to add a user
     *   `GET /users/<id>` endpoint to examine a user in detail
-    *   expanded `POST /users/<id>` endpoint, including password and
+    *   expanded `PATCH /users/<id>` endpoint, including password and
         permissions.
     *   `DELETE /users/<id>` endpoint to delete a user
 *   improved API documentation in [`ref/api.md`](ref/api.md).

README.md

@@ -73,8 +73,8 @@ could use to make this possible:
     *   [UI Development](guide/developing-ui.md)
     *   [Troubleshooting](guide/troubleshooting.md)
 *   [References](ref/), including:
-    *   [Configuration file](refs/config.md)
+    *   [Configuration file](ref/config.md)
-    *   [JSON API](refs/api.md)
+    *   [JSON API](ref/api.md)
 *   [Design documents](design/)
 *   [Wiki](https://github.com/scottlamb/moonfire-nvr/wiki) has hardware
     recommendations, notes on several camera models, etc. Please add more!

ref/api.md

@@ -24,9 +24,9 @@ Status: **current**.
         * [Request 3](#request-3)
     * [User management](#user-management)
         * [`GET /api/users/`](#get-apiusers)
-        * [`PUT /api/users/`](#put-apiusers)
+        * [`POST /api/users/`](#post-apiusers)
         * [`GET /api/users/<id>`](#get-apiusersid)
-        * [`POST /api/users/<id>`](#post-apiusersid)
+        * [`PATCH /api/users/<id>`](#patch-apiusersid)
         * [`DELETE /api/users/<id>`](#delete-apiusersid)
 * [Types](#types)
     * [UserSubset](#usersubset)
@@ -840,9 +840,9 @@ Lists all users. Currently there's no paging. Returns a JSON object with
 a `users` key with an array of objects, each with the following keys:
 
 *   `id`: a number.
-*   `username`: a string.
+*   `user`: a `UserSubset`.
 
-#### `PUT /api/users/`
+#### `POST /api/users/`
 
 Requires the `adminUsers` permission.
 
@@ -858,12 +858,9 @@ Returns status 204 (No Content) on success.
 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 `UserSubset`. The `password`
+Returns a HTTP status 200 on success with a JSON `UserSubset`.
-will be absent (for no password) or a placeholder string to indicate the
-password is set. Passwords are stored hashed, so the cleartext can not be
-retrieved.
 
-#### `POST /api/users/<id>`
+#### `PATCH /api/users/<id>`
 
 Updates the given user. Requires the `adminUsers` permission if the caller is
 not authenticated as the user in question.
@@ -872,8 +869,9 @@ Expects a JSON object:
 
 *   `csrf`: a CSRF token, required when using session authentication.
 *   `update`: `UserSubset`, sets the provided fields. Field-specific notes:
+    *   `username`: requires `adminUsers` permission.
     *   `password`: when updating the password, the previous password must
-        be supplied as a precondition, unless the caller has `admin_users`
+        be supplied as a precondition, unless the caller has `adminUsers`
         permission.
     *   `permissions`: requires `adminUsers` permission. Note that updating a
         user's permissions currently neither adds nor limits permissions of
@@ -901,9 +899,16 @@ Returns HTTP status 204 (No Content) on success.
 
 A JSON object with any of the following parameters:
 
+*   `username`
 *   `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.
+*   `password`
+    *   on retrieval, a placeholder string to indicate a password is set,
+        or null.
+    *   in preconditions, may be left absent to ignore, set to null to require
+        no password, or set to a plaintext string.
+    *   in updates, may be left absent to keep as-is, set to null to disable
+        session creation, or set to a plaintext string.
 *   `permissions`, a `Permissions` as described below.
 
 ### Permissions

server/db/auth.rs

@@ -478,7 +478,11 @@ impl State {
             .context(ErrorKind::Unknown)?;
         }
         let u = e.into_mut();
-        u.username = change.username;
+        if u.username != change.username {
+            self.users_by_name.remove(&u.username);
+            self.users_by_name.insert(change.username.clone(), u.id);
+            u.username = change.username;
+        }
         if let Some(h) = change.set_password_hash {
             u.password_hash = h;
             u.password_id += 1;
@@ -543,7 +547,9 @@ impl State {
         }
         tx.commit().context(ErrorKind::Unknown)?;
         let name = self.users_by_id.remove(&id).unwrap().username;
-        self.users_by_name.remove(&name).unwrap();
+        self.users_by_name
+            .remove(&name)
+            .expect("users_by_name should be consistent with users_by_id");
         self.sessions.retain(|_k, ref mut v| v.user_id != id);
         Ok(())
     }
@@ -1147,6 +1153,34 @@ mod tests {
         );
     }
 
+    #[test]
+    fn change() {
+        testutil::init();
+        let mut conn = Connection::open_in_memory().unwrap();
+        db::init(&mut conn).unwrap();
+        let mut state = State::init(&conn).unwrap();
+        let req = Request {
+            when_sec: Some(42),
+            addr: Some(::std::net::IpAddr::V4(::std::net::Ipv4Addr::new(
+                127, 0, 0, 1,
+            ))),
+            user_agent: Some(b"some ua".to_vec()),
+        };
+        let uid = {
+            let mut c = UserChange::add_user("slamb".to_owned());
+            c.set_password("hunter2".to_owned());
+            state.apply(&conn, c).unwrap().id
+        };
+
+        let user = state.users_by_id().get(&uid).unwrap();
+        let mut c = user.change();
+        c.username = "foo".to_owned();
+        state.apply(&conn, c).unwrap();
+
+        assert!(state.users_by_name.get("slamb").is_none());
+        assert!(state.users_by_name.get("foo").is_some());
+    }
+
     #[test]
     fn delete() {
         testutil::init();

server/src/json.rs

@@ -564,6 +564,17 @@ pub struct UserSubset<'a> {
     pub permissions: Option<Permissions>,
 }
 
+impl<'a> From<&'a db::User> for UserSubset<'a> {
+    fn from(u: &'a db::User) -> Self {
+        Self {
+            username: Some(&u.username),
+            preferences: Some(u.config.preferences.clone()),
+            password: Some(u.has_password().then_some("(censored)")),
+            permissions: Some(u.permissions.clone().into()),
+        }
+    }
+}
+
 // 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<Option<T>, D::Error>
@@ -617,14 +628,14 @@ impl From<db::schema::Permissions> for Permissions {
 
 /// Response to `GET /api/users/`.
 #[derive(Serialize)]
-pub struct GetUsersResponse {
+pub struct GetUsersResponse<'a> {
-    pub users: Vec<UserSummary>,
+    pub users: Vec<UserWithId<'a>>,
 }
 
 #[derive(Serialize)]
-pub struct UserSummary {
+pub struct UserWithId<'a> {
     pub id: i32,
-    pub username: String,
+    pub user: UserSubset<'a>,
 }
 
 /// Response to `PUT /api/users/`.

server/src/web/users.rs

@@ -7,7 +7,7 @@
 use base::{bail_t, format_err_t};
 use http::{Method, Request, StatusCode};
 
-use crate::json::{self, PutUsersResponse, UserSubset, UserSummary};
+use crate::json::{self, PutUsersResponse, UserSubset, UserWithId};
 
 use super::{
     bad_req, extract_json_body, plain_response, require_csrf_if_session, serve_json, Caller,
@@ -18,10 +18,12 @@ impl Service {
     pub(super) async fn users(&self, req: Request<hyper::Body>, caller: Caller) -> ResponseResult {
         match *req.method() {
             Method::GET | Method::HEAD => self.get_users(req, caller).await,
-            Method::PUT => self.put_users(req, caller).await,
+            Method::POST => self.post_users(req, caller).await,
-            _ => Err(
+            _ => Err(plain_response(
-                plain_response(StatusCode::METHOD_NOT_ALLOWED, "GET, HEAD, or PUT expected").into(),
+                StatusCode::METHOD_NOT_ALLOWED,
-            ),
+                "GET, HEAD, or POST expected",
+            )
+            .into()),
         }
     }
 
@@ -29,20 +31,19 @@ impl Service {
         if !caller.permissions.admin_users {
             bail_t!(Unauthenticated, "must have admin_users permission");
         }
-        let users = self
+        let l = self.db.lock();
-            .db
+        let users = l
-            .lock()
             .users_by_id()
             .iter()
-            .map(|(&id, user)| UserSummary {
+            .map(|(&id, user)| UserWithId {
                 id,
-                username: user.username.clone(),
+                user: UserSubset::from(user),
             })
             .collect();
         serve_json(&req, &json::GetUsersResponse { users })
     }
 
-    async fn put_users(&self, mut req: Request<hyper::Body>, caller: Caller) -> ResponseResult {
+    async fn post_users(&self, mut req: Request<hyper::Body>, caller: Caller) -> ResponseResult {
         if !caller.permissions.admin_users {
             bail_t!(Unauthenticated, "must have admin_users permission");
         }
@@ -82,10 +83,10 @@ impl Service {
         match *req.method() {
             Method::GET | Method::HEAD => self.get_user(req, caller, id).await,
             Method::DELETE => self.delete_user(req, caller, id).await,
-            Method::POST => self.post_user(req, caller, id).await,
+            Method::PATCH => self.patch_user(req, caller, id).await,
             _ => Err(plain_response(
                 StatusCode::METHOD_NOT_ALLOWED,
-                "GET, HEAD, DELETE, or POST expected",
+                "GET, HEAD, DELETE, or PATCH expected",
             )
             .into()),
         }
@@ -98,17 +99,7 @@ impl Service {
             .users_by_id()
             .get(&id)
             .ok_or_else(|| format_err_t!(NotFound, "can't find requested user"))?;
-        let out = UserSubset {
+        serve_json(&req, &UserSubset::from(user))
-            username: Some(&user.username),
-            preferences: Some(user.config.preferences.clone()),
-            password: Some(if user.has_password() {
-                Some("(censored)")
-            } else {
-                None
-            }),
-            permissions: Some(user.permissions.clone().into()),
-        };
-        serve_json(&req, &out)
     }
 
     async fn delete_user(
@@ -128,7 +119,7 @@ impl Service {
         Ok(plain_response(StatusCode::NO_CONTENT, &b""[..]))
     }
 
-    async fn post_user(
+    async fn patch_user(
         &self,
         mut req: Request<hyper::Body>,
         caller: Caller,